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

Updates to resample step to account for new L3 metadata #969

Closed
stscijgbot-rstdms opened this issue Nov 3, 2023 · 6 comments
Closed

Updates to resample step to account for new L3 metadata #969

stscijgbot-rstdms opened this issue Nov 3, 2023 · 6 comments

Comments

@stscijgbot-rstdms
Copy link
Collaborator

Issue RCAL-704 was created on JIRA by Nadia Dencheva:

Changes to resample are needed to update newly added L3 attributes.

@stscijgbot-rstdms
Copy link
Collaborator Author

Comment by Jonathan Eisenhamer on JIRA:

Below is a list of issues where guidance/discussion is needed:

The RAD attribute meta.basic.pass cannot exist due to Python syntax issues with the keyword pass

reasample.update_exposure_times, there is no schema for "total exposure time"

resample_test:479 no longer any equivalence to exposure_time

conversion to flux is wrong, see resample.py:417

There is no longer any schema for a usable WCS

The WCS issue is easily resolved; just need to add back in the schema that will create the basic FITS-like wcs information, and maybe the GWCS object itself. Just need the go-ahead for this.

For the exposure time reporting, if the way the resample step calculates all this is acceptable, it should be easy enough to add back in or use what exists in the meta.resample. Just need the go-ahead for this.

How/when to convert to flux will need guidance.

What to rename the pass attribute to will need guidance.

@schlafly
Copy link
Collaborator

schlafly commented Jan 2, 2024

Thanks Jonathan. Ugh, the pass thing is annoying. The related term we use in DESI is "layer", but we were trying to adopt the same terms that were used in APT. Maybe pass_plan; let's discuss on Thursday.

This is intended to be the new total exposure time:
https://github.com/spacetelescope/rad/pull/334/files#diff-ffe0e1cde69572f25d2f4adc6b8e59de2f31aaf6e7f5314e33f381c4230363a6R50
and this an equivalent maximum
https://github.com/spacetelescope/rad/pull/334/files#diff-ffe0e1cde69572f25d2f4adc6b8e59de2f31aaf6e7f5314e33f381c4230363a6R40

i.e., the median here should get replaced by mean
https://github.com/spacetelescope/romancal/blob/main/romancal/resample/resample.py#L480-L481
and these should go into mean_exposure_time and max_exposure_time.

Possibly just delete this test
https://github.com/spacetelescope/romancal/blob/main/romancal/resample/tests/test_resample.py#L476-L482
IIRC these three exposures all overlap, so that the maximum exposure time is 3x nominal, but the mean depends on the details of the overlap and is too complicated to put into a unit test. For the current test we were using the median which happened to be 2x nominal which was easy enough.

Re units, I think to get this working we need to multiply the L2 images by meta.photometry.conversion_megajansky, and the variances that get sent to drizzle by the square of that. I think some care is going to be needed to get that right; we resample at least a few different times

  • resample_many_to_one
  • resample_many_to_many
  • resample_variance_array
  • resample_exposure_time

and each of these read in the images separately. I'm not immediately seeing a good entrance point to avoid duplicating code among those four paths to convert things to MJy/sr units. Probably code around here
https://github.com/spacetelescope/romancal/blob/main/romancal/resample/resample.py#L212-L228
to check if the image is in e/s and convert multiply data by photometry['conversion_megajansky'] if so, and likewise code in build_driz_weight to multiply the inwht by 1/conversion_megajansky^2 if ivm is used. And then likewise multiplying variance by conversion_megajansky^2 if it's in e/s here
https://github.com/spacetelescope/romancal/blob/main/romancal/resample/resample.py#L359
That would likely also break unit tests, etc., though we could presumably add the photometry block to the meta in the unit tests with conversion_megajansky = 1 to preserve those?

I'm not really sure what's meant by the schema for a usable WCS. Is this wcsinfo in the old version? Yes, we would plan to include a gwcs object that isn't explicitly listed in the schema, though I think that's the case for the L2 images as well.

@stscijgbot-rstdms
Copy link
Collaborator Author

stscijgbot-rstdms commented Jan 19, 2024

Comment by Jonathan Eisenhamer on JIRA:

I will be setting the PR's towards Paul Huwe to read-for-review. This will leave the following:

  • The change of pass to pass_number breaks all test data. That will need resolution.
  • The wcs issue is handled in rad PR#⁠351. Will need to test romancal against this (unless already done)
  • After discussion with Eddie Schlafly, the flux issue be handled through a separate issue. The current state of the units in the L3 meta will remain.

Wondering if a different branch in the primary romancal repo should be created due to the number of PRs and the state of the regression testing and need for flux calibration to be completed?

@stscijgbot-rstdms
Copy link
Collaborator Author

Comment by Paul Huwe on JIRA:

To clarify: does "breaks all test data" refer to data files used in regtests or to unit tests as well? The former is expected (we have several RAD / RDM changes that will require updates to regtest artifacts). 

@stscijgbot-rstdms
Copy link
Collaborator Author

Comment by Jonathan Eisenhamer on JIRA:

Sorry, yes, the regressions. Figured it was going to be expected.

@stscijgbot-rstdms
Copy link
Collaborator Author

Comment by Jonathan Eisenhamer on JIRA:

Resolved by rcal PR#⁠1057, rdm PR#⁠288, and rad PR#⁠334

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

No branches or pull requests

2 participants