Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

changing name of segmentation map flux keyword #717

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

eas342
Copy link
Collaborator

@eas342 eas342 commented Aug 27, 2021

Updating the docs to the right keyword to use in the yaml generator for setting a flux limit for the segmentation map threshold.

@bhilbert4
Copy link
Collaborator

I think the minimum_signal and minimum_signal_units are correct in this case. You can see in the call to yaml_generator, that it contains:
segmap_flux_limit=minmum_signal, segmap_flux_limit_units=minimum_signal_units

I see that there is a typo in that minimum_signal is misspelled, but I think that's the only change that's necessary.

@eas342
Copy link
Collaborator Author

eas342 commented Aug 27, 2021

Hmm, maybe I'm looking at the wrong branch or misinterpreted code but when I go here:
https://github.com/spacetelescope/mirage/blob/master/mirage/yaml/yaml_generator.py

line 136 says:
dateobs_for_background=False, segmap_flux_limit=None, segmap_flux_limit_units=None,

And line 326 says:
self.segmentation_threshold_units = segmap_flux_limit_units

@eas342
Copy link
Collaborator Author

eas342 commented Aug 27, 2021

Ah got you! I didn't read all the way down at the bottom of the page where you call yaml_generator.SimInput and convert it.

@bhilbert4
Copy link
Collaborator

Right. minimum_signal and minimum_signal_units are just random variable names that are used as inputs. Maybe I should have put a call to yaml_generator in each section of that page, to cut down on confustion.

@eas342
Copy link
Collaborator Author

eas342 commented Aug 27, 2021

Ok, I updated this pull request to use the same segmap_flux_limit in both the description and the call to yaml_generator.SimInput for lazy people like me who can't read the whole page for find the correct docstring for the function.

Up to you how to you see fit to proceed

@bhilbert4
Copy link
Collaborator

Actually, I think the only change needed is:
segmap_flux_limit=minmum_signal -> segmap_flux_limit=minimum_signal

@bhilbert4
Copy link
Collaborator

Or yeah, you could change all instances of minimum_signal to segmap_flux_limit, and minimum_signal_units to segmap_flux_limit_units.
True, that would probably help avoid confusion

@eas342
Copy link
Collaborator Author

eas342 commented Aug 27, 2021

Maybe for lazy people who don't read the whole thing like me, but you are correct and fair with segmap_flux_limit=minmum_signal -> segmap_flux_limit=minimum_signal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants