spacetelescope / jwst

Python library for science observations from the James Webb Space Telescope
https://jwst-pipeline.readthedocs.io/en/latest/
Other
558 stars 164 forks source link

Skymatch documentation for "global+match" not clear #6581

Closed stscijgbot-jp closed 2 years ago

stscijgbot-jp commented 2 years ago

Issue JP-2387 was created on JIRA by Mattia Libralato:

The documentation of the skymatch step regarding the option "global+match" is not clear. Regardless whether "subtract" is True or False, the output i2d image of the resample step has the background subtracted.

The current documentation says: 

’global+match’ : first use ‘match’ method to equalize sky values between images and then find a minimum “global” sky value in all input images.
Note
This is the recommended setting for images containing diffuse sources (e.g., galaxies, nebulae) covering significant parts of the image.

The "match" option already matches the background to that of the minimum (or maximum depending on the "match_down" option) background among all images.

 

Furthermore, the readthedocs documentation also writes:

The 'global+match' algorithm combines 'match' and 'global' methods in order to overcome the limitation of the 'match' method described in the note above: it uses 'global' algorithm to find a baseline sky value common to all input images and the 'match' algorithm to “equalize” sky values in the mosaic. Thus, the sky value of the “reference” image will be equal to the baseline sky value (instead of 0 in 'match' algorithm alone).

Because of how the pipeline is designed (see comments in JP-2024, when using "global+match" the variable that stores the background of each image initially saves the bkg difference (with "match") and then the code adds to it the minimum background among all images (with "global"). Regardless if "subtract" is True or False, the final i2d resampled image has the background subtracted. If you use only "match", the background in the final image is equal to the minimum (or maximum) bkg value among all images. Thus, I would say that the last sentence is not correct.

 

I think the documentation should better describe what this option allows the user to do, and what are the differences with the "match" option.

stscijgbot-jp commented 1 year ago

Comment by Howard Bushouse on JIRA:

"Regardless if "subtract" is True or False, the final i2d resampled image has the background subtracted."

Yes, that's because the final i2d resampled image is created by the "resample" step, which is completely separate from the "skymatch" step, and the "subtract" param is for the "skymatch" step only. If "skymatch.subtract=False", which is the default, the images coming out of the skymatch step will not have any background subtracted.

It's not until you then feed those images into the "resample" step, to produce a combined product, that background subtraction is happening if the background was not already subtracted by "skymatch".

stscijgbot-jp commented 1 year ago

Comment by Howard Bushouse on JIRA:

Regardless, we will try to clean up the docs a bit to make the interaction between "skymatch" and "resample" a bit more obvious.

stscijgbot-jp commented 1 year ago

Comment by Mattia Libralato on JIRA:

There is another part that should be clarified: what is the difference between 'global+match' and 'match' with 'match_down=True'.

stscijgbot-jp commented 1 year ago

Comment by Howard Bushouse on JIRA:

Will do.

stscijgbot-jp commented 1 year ago

Comment by Mattia Libralato on JIRA:

Thanks!

stscijgbot-jp commented 1 year ago

Comment by Howard Bushouse on JIRA:

In off-line conversation it has also been agreed to add another keyword to the skymatch output files, called "BKGMETH", which will record what method was used to do the sky computation (as information to users).

stscijgbot-jp commented 1 year ago

Comment by Howard Bushouse on JIRA:

Fixed in #6726

stscijgbot-jp commented 1 year ago

Comment by Misty Cracraft on JIRA:

Mattia Libralato Could you check over the documentation and see if it makes more sense now?

stscijgbot-jp commented 1 year ago

Comment by Mattia Libralato on JIRA:

Apologies, I thought this was solved since I already gave my comments on Github. Anyway, I read everything again and I confirm that the new text is fine for me.

Howard Bushouse There might be one inconsistency between the description of the method 'global' in the 'Description' page and in the 'skymatch' page. In the former, the text says that with 'global' the code initially uses the 'local' method to estimate the sky, while in the latter it says that the code uses 'match'.

Finally, a very minor note. In the 'Step Arguments' page, the 'skystat' description says "Supported values are ‘mean’, ‘mode’, ‘midpt’, and ‘median’." If you want to keep the text with a uniform style, I think it should be better to write mean, mode, midpt and median with that white-ish box style as, for example, when describing the skymethod options.

stscijgbot-jp commented 1 year ago

Comment by Misty Cracraft on JIRA:

So Mattia Libralato  are you asking for more updates, in which case we should send this back to SCSB to add to the documentation, or are you giving the ok to close the ticket?

stscijgbot-jp commented 1 year ago

Comment by Mattia Libralato on JIRA:

I do not know that is the procedure to follow here. The first point raised is a potential inconsistency, the second one is just a style comment, which can be disregarded. If Howard Bushouse thinks that the first point needs to be fixed, then I would not close the ticket. If not, you can close the ticket.

stscijgbot-jp commented 1 year ago

Comment by Howard Bushouse on JIRA:

Made these final two tweaks to the docs in https://github.com/spacetelescope/jwst/pull/6891, which will be included in B8.1.

stscijgbot-jp commented 1 year ago

Comment by Misty Cracraft on JIRA:

I am satisfied all suggested changes have been made.