Skip to content

changing name of segmentation map flux keyword - #717

Open
eas342 wants to merge 2 commits into
spacetelescope:masterfrom
eas342:updateSegThreshInDoc
Open

changing name of segmentation map flux keyword#717
eas342 wants to merge 2 commits into
spacetelescope:masterfrom
eas342:updateSegThreshInDoc

Conversation

@eas342

@eas342 eas342 commented Aug 27, 2021

Copy link
Copy Markdown
Collaborator

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
Copy Markdown
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

eas342 commented Aug 27, 2021

Copy link
Copy Markdown
Collaborator Author

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

eas342 commented Aug 27, 2021

Copy link
Copy Markdown
Collaborator Author

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
Copy Markdown
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

eas342 commented Aug 27, 2021

Copy link
Copy Markdown
Collaborator Author

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
Copy Markdown
Collaborator

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

@bhilbert4

Copy link
Copy Markdown
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

eas342 commented Aug 27, 2021

Copy link
Copy Markdown
Collaborator Author

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