-
Notifications
You must be signed in to change notification settings - Fork 28
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
Use dqflags defined in roman_datamodels #1099
Use dqflags defined in roman_datamodels #1099
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1099 +/- ##
==========================================
- Coverage 76.81% 76.78% -0.04%
==========================================
Files 105 105
Lines 7048 7047 -1
==========================================
- Hits 5414 5411 -3
- Misses 1634 1636 +2
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Sentry. |
This does not effect the regression tests in any way, see: https://plwishmaster.stsci.edu:8081/job/RT/job/Roman-Developers-Pull-Requests/600/. All that has happened is the dqflag definitions have been moved to |
3d73827
to
39ff54c
Compare
This looks good to me. It looks like 0.14.0 is in "draft" state, but we could merge after that actually comes out? |
ba78573
to
294b15a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why we need this change. In general the dq flags are tightly coupled with the calibration process and it make sense to keep these together.
This also seems to be counter to most other calibration software that keeps the dq flags and calibration software together.
So what problem is this solving?
Here are a couple of use cases:
|
294b15a
to
85c7ea7
Compare
I'm not sure why you want to single out the dq values? You already put in add hoc values that are defined elsewhere, e.g. the read pattern is defined in the PRD. Are you also proposing that be added to the data models? By moving the dq flags to roman_datamodels then you must have roman_datamodels for the cal code. In general this is not a problem but if someone (SSC?) wants to write a custom i/o package to read their data them you are still requiring them to install roman_datamodels to get the pipeline to work. Note: we do have a requirement to support custom data processing from the users (SOC-1061) For a user to understand the dq flags we have a page specifically for to address this (https://roman-pipeline.readthedocs.io/en/latest/roman/dq_init/reference_files.html) The cal pipeline is also the place where these values are "defined", The dqflags.py is really only a dictionary to allow you to use the names in place of a value. The real definition for these, what they do and how they are used, is encoded in the cal pipeline code. So I'm not sure that the dq flags should be separate from the cal code. |
I do not want to propose that we have read_pattern defined in roman_datamodels. I see roman_datamodels as something that lets people read and interact with data we produce. It's natural for the DQ flags to live there since otherwise to be able to use names for the DQ bits you need to copy the definitions from the documentation. I agree that you can do that; the point of this is to allow people to avoid making those copies. I also agree that there's another approach where rad, rdm, and romancal are one package. But insofar as we have decided to separate ~metadata and structures from algorithms, the names of the dq bits look to me to fit better with the metadata. Certainly the simulator would use these, and I anticipate that other code would as well. I agree that other people will need to install some dependency in order to get to the flag definitions. The statement is just that roman_datamodels is a smaller dependency than the full pipeline, and that it's one that many users of our data products will want to install, since its whole purpose is to allow people to interact with the data files we produce. |
85c7ea7
to
4233f35
Compare
This passes regtests: https://plwishmaster.stsci.edu:8081/job/RT/job/Roman-Developers-Pull-Requests/617/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, merging.
This PR updates romancal to take advantage of the changes in spacetelescope/roman_datamodels#293.
Checklist
CHANGES.rst
under the corresponding subsection