-
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
Rcal 596 Add association processing to the pipeline code #802
Conversation
f872848
to
e965b24
Compare
FYI: doc failure appears related to an old theme version, similar to: https://github.com/spacetelescope/jwst/pull/7787/files |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #802 +/- ##
==========================================
- Coverage 76.95% 75.96% -1.00%
==========================================
Files 94 96 +2
Lines 5690 5799 +109
==========================================
+ Hits 4379 4405 +26
- Misses 1311 1394 +83
*This pull request uses carry forward flags. Click here to find out more.
☔ View full report in Codecov by Sentry. |
e1aa6f9
to
13869c6
Compare
Thanks Brett, that fixed the problem! |
25951b7
to
13869c6
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.
Looks good to me. I left a few comments in line in the pipeline stage where I didn't understand the logic.
Do the regression tests really take 2 hours to run on your system? Is that just the difference between your system and plwishmaster (~20 min, IIRC?)?
Yes, Mostly downloading lots of large files.
else: | ||
log.info("Flat Field step is being SKIPPED") | ||
result.meta.cal_step.flat_field = "SKIPPED" | ||
return results |
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'm confused about this. The old version had this line under the is_fully_saturated condition; here this is outside that condition, so if there is ' groupdq' in result.keys() we stop the pipeline? I think I would have guessed that this should be indented one block, and that rather than returning, we continue to the next SCA?
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.
That looks like a rebase/merge issue and I'll fix that.
|
||
if result.meta.exposure.type == "WFI_IMAGE": | ||
result = self.photom(result) | ||
result = self.source_detection(result) |
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.
This might be a merge / rebase issue or something, but I think we want to keep this "only do source detection for images" condition?
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.
Yes that must have been wiped out.
The check is also in the steps so it doesn't matter too much. The logic is here to avoid extra setup.
self.setup_output(result) | ||
log.info("Roman exposure calibration pipeline ending...") | ||
|
||
self.output_use_model = True |
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 didn't see where this was used.
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.
This is used in stpipe to set the output file names.
d98fd5c
to
9dae67e
Compare
Updates for logging and step skips for non WFI_IMAGE data. The pipeline now produces with imaging and spectral data,
and for the spectral data file
|
d3d9677
to
e9ddba6
Compare
|
||
Returns | ||
------- | ||
association : DMSLevel2bBase |
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.
Should we get away from JWST names and levels, since there's noLevel 2b for Roman?
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.
That's fine. I did this for expediency and when the associations were much more poorly defined. Can this be a separate ticket to clean this up?
if isinstance(obj, str): | ||
file_name, file_ext = os_path.splitext(obj) | ||
|
||
if file_ext == ".fits": |
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.
And what about not using FITS files or do you see this code being combined in the future with the code for JWST?
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 kept this to cover a case if/when someone will give romancal a fits file. We can just give a warning and gracefully exit.
There is also the type error in roman_datamodels (TypeError: Roman datamodels does not accept FITS files or objects) should we stick with that?
try: | ||
input = rdm.open(input) | ||
except TypeError: | ||
log.debug("Error opening file:") |
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.
What should happen in the case of one asdf file which cannot be opened? Should it return?
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 guess the question is, if any of the inputs in the association fail for any reason will the end user still want the results?
My choice would be to stop and let the user know. They can fix the error and rerun the pipeline. That way they make the decision if the results are valid without all the inputs. Not very pythonic of me.
try: | ||
asn = LoadAsLevel2Asn.load(input, basename=self.output_file) | ||
except AssociationNotValidError: | ||
log.debug("Error opening file:") |
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.
Similarly here - if an association is invalid shouldn't the code exit?
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.
Is there a way to make sure it's a Level 2 asn, to prevent running the code on L2 products? Even if this is desirable I'm not asking to be done in this PR.
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 left all that out. I'd like to address that once rcal-620 and 621 are in. I think that would be a better place to enforce validity.
But yes here the code should just exit.
5e68e6b
to
435b75c
Compare
9c7bfae
to
11ff6ba
Compare
5b82460
to
19496c9
Compare
That is not actually true. In some cases you will only want one result.
For this case you should get a list of results.
Updated the code.
…On 8/4/23 10:05 AM, Eddie Schlafly wrote:
***@***.**** commented on this pull request.
------------------------------------------------------------------------
In romancal/pipeline/exposure_pipeline.py
<https://urldefense.com/v3/__https://github.com/spacetelescope/romancal/pull/802*discussion_r1284469655__;Iw!!CrWY41Z8OgsX0i-WU-0LuAcUu2o!wHxyUKS7hhumUaUEsyUXPN-bGrPcBJD5lhUYk3VfIbwDViKm09zir4rctOTIrZkmz7842lOUAaqZMVpSX3CUER-i$>:
> if is_fully_saturated(result):
- # Set all subsequent steps to skipped
- for step_str in [
- "assign_wcs",
- "flat_field",
- "photom",
- "source_detection",
- ]:
- result.meta.cal_step[step_str] = "SKIPPED"
-
- # Set suffix for proper output naming
- self.suffix = "cal"
+ log.info("All pixels are saturated. Returning a zeroed-out image.")
+
+ # Return zeroed-out image file (stopping pipeline)
+ return self.create_fully_saturated_zeroed_image(result)
I hear you that some of this could be a different PR, but at least the
bit where we still want to process all of the images even if the first
one is all saturated applies? i.e., replace return
self.create_fully_saturated_zeroed_image with
results.append(self.create_fully_saturated_zeroed_image) ; continue,
right? The step should always return a list of results if fed an
association, and never just one?
—
Reply to this email directly, view it on GitHub
<https://urldefense.com/v3/__https://github.com/spacetelescope/romancal/pull/802*discussion_r1284469655__;Iw!!CrWY41Z8OgsX0i-WU-0LuAcUu2o!wHxyUKS7hhumUaUEsyUXPN-bGrPcBJD5lhUYk3VfIbwDViKm09zir4rctOTIrZkmz7842lOUAaqZMVpSX3CUER-i$>,
or unsubscribe
<https://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/ALXCXWPG36SQAZWFTNCB2BLXTT6UNANCNFSM6AAAAAA3AFONEE__;!!CrWY41Z8OgsX0i-WU-0LuAcUu2o!wHxyUKS7hhumUaUEsyUXPN-bGrPcBJD5lhUYk3VfIbwDViKm09zir4rctOTIrZkmz7842lOUAaqZMVpSX1MBg03r$>.
You are receiving this because you were assigned.Message ID:
***@***.***>
|
1477775
to
5c316fd
Compare
0b94ad9
to
d4ed5f4
Compare
for more information, see https://pre-commit.ci
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'm still confused about the intended behavior in the fully saturated case. I've added code suggestions below and am approving. Thanks!
log.info("Roman exposure calibration pipeline ending...") | ||
|
||
return result | ||
return results |
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.
return results | |
continue |
Indenting and changing return -> continue so other detectors will still process.
|
||
# Set suffix for proper output naming | ||
self.suffix = "cal" | ||
results.append(result) | ||
|
||
# Return fully saturated image file (stopping pipeline) |
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.
# Return fully saturated image file (stopping pipeline) | |
# Skip remaining steps on this detector. |
Indenting this to go under the saturation check, and updating the comment.
I don't understand your objections. When there is a fully saturated
input file I get
2023-08-07 10:31:44,768 - stpipe.ExposurePipeline.source_detection -
INFO - Step source_detection done
2023-08-07 10:31:44,768 - stpipe.ExposurePipeline - INFO - Roman
exposure calibration pipeline ending...
2023-08-07 10:31:46,735 - stpipe.ExposurePipeline - INFO - Saved model
in r0000101001001001001_01101_0001_WFI01_cal.asdf
2023-08-07 10:31:48,481 - stpipe.ExposurePipeline - INFO - Saved model
in r0000101001001001001_01101_0001_WFI01_sat_cal.asdf
2023-08-07 10:31:50,110 - stpipe.ExposurePipeline - INFO - Saved model
in r0000101001001001001_01101_0001_WFI02_cal.asdf
2023-08-07 10:31:50,110 - stpipe.ExposurePipeline - INFO - Step
ExposurePipeline done
So the unsaturated data is calibrated and output and the saturated data
output is also output.
and the saturated data file has 0's as expected.
np.max(sat.data)
<Quantity 0. electron / s>
…On 8/7/23 10:03 AM, Eddie Schlafly wrote:
***@***.**** approved this pull request.
I'm still confused about the intended behavior in the fully saturated
case. I've added code suggestions below and am approving. Thanks!
------------------------------------------------------------------------
In romancal/pipeline/exposure_pipeline.py
<https://urldefense.com/v3/__https://github.com/spacetelescope/romancal/pull/802*discussion_r1285907756__;Iw!!CrWY41Z8OgsX0i-WU-0LuAcUu2o!x0q7ttMj6nmthy1hN0uNxFfvgHcpl4Xvy9DIr1eEe-7PWuI5BxeqNfApTwxdBO-62RN8nhxHQCk8ffyI7DOxjU8D$>:
> - result.meta.cal_step.flat_field = "SKIPPED"
-
- result = self.photom(result)
-
- if result.meta.exposure.type == "WFI_IMAGE":
- result = self.source_detection(result)
- else:
- log.info("Source Detection step is being SKIPPED")
- result.meta.cal_step.source_detection = "SKIPPED"
-
- # setup output_file for saving
- self.setup_output(result)
- log.info("Roman exposure calibration pipeline ending...")
-
- return result
+ return results
⬇️ Suggested change
- return results
+ continue
Indenting and changing return -> continue so other detectors will
still process.
------------------------------------------------------------------------
In romancal/pipeline/exposure_pipeline.py
<https://urldefense.com/v3/__https://github.com/spacetelescope/romancal/pull/802*discussion_r1285908434__;Iw!!CrWY41Z8OgsX0i-WU-0LuAcUu2o!x0q7ttMj6nmthy1hN0uNxFfvgHcpl4Xvy9DIr1eEe-7PWuI5BxeqNfApTwxdBO-62RN8nhxHQCk8ffyI7Bi5lkRq$>:
> +
+ # Test for fully saturated data
+ if "groupdq" in result.keys():
+ if is_fully_saturated(result):
+ # Set all subsequent steps to skipped
+ for step_str in [
+ "assign_wcs",
+ "flat_field",
+ "photom",
+ "source_detection",
+ ]:
+ result.meta.cal_step[step_str] = "SKIPPED"
+
+ # Set suffix for proper output naming
+ self.suffix = "cal"
+ results.append(result)
# Return fully saturated image file (stopping pipeline)
⬇️ Suggested change
- # Return fully saturated image file (stopping pipeline)
+ # Skip remaining steps on this detector.
Indenting this to go under the saturation check, and updating the comment.
—
Reply to this email directly, view it on GitHub
<https://urldefense.com/v3/__https://github.com/spacetelescope/romancal/pull/802*pullrequestreview-1565395332__;Iw!!CrWY41Z8OgsX0i-WU-0LuAcUu2o!x0q7ttMj6nmthy1hN0uNxFfvgHcpl4Xvy9DIr1eEe-7PWuI5BxeqNfApTwxdBO-62RN8nhxHQCk8ffyI7DG95Bi4$>,
or unsubscribe
<https://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/ALXCXWKLFVKNCKKT4ZVQZ6DXUDYT3ANCNFSM6AAAAAA3AFONEE__;!!CrWY41Z8OgsX0i-WU-0LuAcUu2o!x0q7ttMj6nmthy1hN0uNxFfvgHcpl4Xvy9DIr1eEe-7PWuI5BxeqNfApTwxdBO-62RN8nhxHQCk8ffyI7E2yn-9v$>.
You are receiving this because you were assigned.Message ID:
***@***.***>
|
Okay, I guess I'm misunderstanding the code, go ahead. |
Sure. If this is a corner case or something I'm missing let's just write a
ticket for an improvement.
…On 8/7/23 11:11 AM, Eddie Schlafly wrote:
Okay, I guess I'm misunderstanding the code, go ahead.
—
Reply to this email directly, view it on GitHub
<https://urldefense.com/v3/__https://github.com/spacetelescope/romancal/pull/802*issuecomment-1668061033__;Iw!!CrWY41Z8OgsX0i-WU-0LuAcUu2o!xBKWSOfJOLg1pWDZlOF_yIKgb0590jFzvAuQ07SQ6xbdhBUX2Mkjbd1KVodUlg2VQ-zw_NnuyiSj7Zh66qlQJBKd$>,
or unsubscribe
<https://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/ALXCXWOW5T4TZP5MZWXJDXDXUEAR3ANCNFSM6AAAAAA3AFONEE__;!!CrWY41Z8OgsX0i-WU-0LuAcUu2o!xBKWSOfJOLg1pWDZlOF_yIKgb0590jFzvAuQ07SQ6xbdhBUX2Mkjbd1KVodUlg2VQ-zw_NnuyiSj7Zh66qvVFTn2$>.
You are receiving this because you were assigned.Message ID:
***@***.***>
|
'rad @ git+https://github.com/spacetelescope/rad.git@main', | ||
# 'roman_datamodels >=0.15.0', | ||
'roman_datamodels @ git+https://github.com/spacetelescope/roman_datamodels.git@main', | ||
'rad == 0.17.0', |
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.
The devdeps job on plwishmaster is currently installing rad and roman_datamodels 0.17.0 instead of the dev versions:
https://plwishmaster.stsci.edu:8081/blue/organizations/jenkins/RT%2FRoman-devdeps/detail/Roman-devdeps/361/pipeline/176
I think this pin is causing the devdeps job on plwishmaster to pull the older, 0.17.0 rad (and similar for roman_datamodels) instead of testing against the 0.17.1 (or development version, which I think is the same at the moment).
@ddavis-stsci was this a requirement for this PR? If so, is there a follow-up PR or tracking issue to remove the pin?
The pin is needed until we get new input files that match the latest RAD
and roman_datamodels.
Once we have that we'll switch back to the latest released version of
both packages.
…On 8/7/23 4:19 PM, Brett Graham wrote:
***@***.**** commented on this pull request.
------------------------------------------------------------------------
In pyproject.toml
<https://urldefense.com/v3/__https://github.com/spacetelescope/romancal/pull/802*discussion_r1286348990__;Iw!!CrWY41Z8OgsX0i-WU-0LuAcUu2o!zhaTuljbhJSosDCuhH6HTdov51V2a2_HOpD7OcFIYKFJ_d8tKiQz4hElhSwEP2Lrq9XwS7xd-C1XDuK3y-gJBLYy$>:
> @@ -22,10 +22,11 @@ dependencies = [
'photutils >=1.6.0',
'pyparsing >=2.4.7',
'requests >=2.22',
- # 'rad >=0.15.0',
- 'rad @ ***@***.***',
- # 'roman_datamodels >=0.15.0',
- 'roman_datamodels @ ***@***.***',
+ 'rad == 0.17.0',
I think this pin is causing the devdeps job on plwishmaster to pull
the older, 0.17.0 rad (and similar for roman_datamodels) instead of
testing against the 0.17.1 (or development version, which I think is
the same at the moment).
@ddavis-stsci
<https://urldefense.com/v3/__https://github.com/ddavis-stsci__;!!CrWY41Z8OgsX0i-WU-0LuAcUu2o!zhaTuljbhJSosDCuhH6HTdov51V2a2_HOpD7OcFIYKFJ_d8tKiQz4hElhSwEP2Lrq9XwS7xd-C1XDuK3y2BxXenk$>
was this a requirement for this PR? If so, is there a follow-up PR or
tracking issue to remove the pin?
—
Reply to this email directly, view it on GitHub
<https://urldefense.com/v3/__https://github.com/spacetelescope/romancal/pull/802*pullrequestreview-1566093493__;Iw!!CrWY41Z8OgsX0i-WU-0LuAcUu2o!zhaTuljbhJSosDCuhH6HTdov51V2a2_HOpD7OcFIYKFJ_d8tKiQz4hElhSwEP2Lrq9XwS7xd-C1XDuK3y6qm5S7b$>,
or unsubscribe
<https://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/ALXCXWPWR6KVYJQC7CJDBWTXUFEVFANCNFSM6AAAAAA3AFONEE__;!!CrWY41Z8OgsX0i-WU-0LuAcUu2o!zhaTuljbhJSosDCuhH6HTdov51V2a2_HOpD7OcFIYKFJ_d8tKiQz4hElhSwEP2Lrq9XwS7xd-C1XDuK3y-vSzd9E$>.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Thanks! That explains why 22 of the 23 failing tests fixed themselves :) |
'rad == 0.17.0', | ||
# 'rad >= 0.16.0, < 0.17.1', | ||
# 'rad @ git+https://github.com/spacetelescope/rad.git@main', | ||
'roman_datamodels ==0.17.0', | ||
# 'roman_datamodels @ git+https://github.com/spacetelescope/roman_datamodels.git@main', |
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.
There absolutely should not be exact pins for these dependencies. This will cause us major headaches in development.
…ope#802) Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Resolves RCAL-596
Closes #755
This PR adds the initial association processing to the pipeline. This is an initial implementation and once rcal-620 and rcal-621 are done we will use ModelContainer and the open function to load the association.
The current usage requires the use of disable-crds-steppar option but should be relaxed on the two Jira issues above are
addressed. To run the pipeline with an association the command is,
strun --disable-crds-steppar roman_elp r90001-a3001_image_001_asn.json
Checklist
CHANGES.rst
under the corresponding subsectionpytest romancal/tests/
=============================================================== test session starts ================================================================
platform darwin -- Python 3.11.0, pytest-7.4.0, pluggy-1.2.0
rootdir: /Users/ddavis/src/Roman/romancal
configfile: pyproject.toml
plugins: remotedata-0.4.0, asdf-2.15.0, hypothesis-6.82.0, doctestplus-0.13.0, cov-4.1.0, filter-subpackage-0.1.2, mock-3.11.1, astropy-header-0.2.2, astropy-0.10.0, ci-watson-0.6.1, openfiles-0.5.0, env-0.8.2, arraydiff-0.5.0
collected 92 items
romancal/tests/test_import.py ............................................................................................ [100%]
================================================================ 92 passed in 1.59s ================================================================
(rcal_dev) bash>pytest romancal/datamodels/tests/
=============================================================== test session starts ================================================================
platform darwin -- Python 3.11.0, pytest-7.4.0, pluggy-1.2.0
rootdir: /Users/ddavis/src/Roman/romancal
configfile: pyproject.toml
plugins: remotedata-0.4.0, asdf-2.15.0, hypothesis-6.82.0, doctestplus-0.13.0, cov-4.1.0, filter-subpackage-0.1.2, mock-3.11.1, astropy-header-0.2.2, astropy-0.10.0, ci-watson-0.6.1, openfiles-0.5.0, env-0.8.2, arraydiff-0.5.0
collected 42 items
romancal/datamodels/tests/test_datamodels.py ......................................... [ 97%]
romancal/datamodels/tests/test_filetype.py . [100%]
=============================================================== 42 passed in 23.78s
Since plwishmaster is down I ran the regression tests locally,
platform darwin -- Python 3.11.0, pytest-7.4.0, pluggy-1.2.0
rootdir: /Users/ddavis/src/Roman/romancal
configfile: pyproject.toml
plugins: remotedata-0.4.0, asdf-2.15.0, hypothesis-6.82.0, doctestplus-0.13.0, cov-4.1.0, filter-subpackage-0.1.2, mock-3.11.1, astropy-header-0.2.2, astropy-0.10.0, ci-watson-0.6.1, openfiles-0.5.0, env-0.8.2, arraydiff-0.5.0
collected 21 items
../../../romancal/romancal/regtest/test_dark_current.py .... [ 19%]
../../../romancal/romancal/regtest/test_jump_det.py . [ 23%]
../../../romancal/romancal/regtest/test_linearity.py .. [ 33%]
../../../romancal/romancal/regtest/test_ramp_fitting.py . [ 38%]
../../../romancal/romancal/regtest/test_refpix.py . [ 42%]
../../../romancal/romancal/regtest/test_tweakreg.py . [ 47%]
../../../romancal/romancal/regtest/test_wfi_dq_init.py .. [ 57%]
../../../romancal/romancal/regtest/test_wfi_flat_field.py ... [ 71%]
../../../romancal/romancal/regtest/test_wfi_photom.py . [ 76%]
../../../romancal/romancal/regtest/test_wfi_pipeline.py ... [ 90%]
../../../romancal/romancal/regtest/test_wfi_saturation.py .. [100%]
...
-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
================= 21 passed, 19 warnings in 7665.81s (2:07:45)