RCAL-661/667/654/656 Implement next round of SOC verification tests for uneven ramps #970
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #970 +/- ##
==========================================
- Coverage 70.97% 70.94% -0.04%
==========================================
Files 104 105 +1
Lines 6973 6979 +6
==========================================
+ Hits 4949 4951 +2
- Misses 2024 2028 +4
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Sentry. |
romancal/lib/dms.py
Outdated
| from stpipe import log as stpipe_log | ||
|
|
||
|
|
||
| def result_logger(requirement, message): |
There was a problem hiding this comment.
I thought the metrics_logger package already did this for us, and we shouldn't need to add anything?
There was a problem hiding this comment.
This was what generated my question on slack about the double-logging, which is seen in other tests. If there is no particular need for this, I am happy to pull this out and rely simply on the provided metrics logger.
There was a problem hiding this comment.
Let's use the metrics_logger as in the other tests.
| @@ -23,7 +23,7 @@ def passfail(bool_expr): | |||
|
|
|||
| @pytest.mark.bigdata | |||
| @pytest.mark.soctests | |||
| @metrics_logger("DMS86", "DMS87", "DMS88") | |||
| @metrics_logger("DMS86", "DMS87", "DMS88", "DMS280") | |||
There was a problem hiding this comment.
Did we have any other DMS requirements we wanted to close as part of this? I'm looking for the DMS362, 366, 363, 367? Or you're waiting to plug in the data before decorating those?
There was a problem hiding this comment.
If I have read them correctly, which is possibly not, DMS362, 366, 363, and 367, all relate to just the ramp fit step, which are now all handled in the test_ramp_fit.py test module, which has just been pushed.
There was a problem hiding this comment.
Ah, good, I see this now. We need to decorate the tests with the metrics_logger decoration so that whatever tools pick up the tests right. It's not clear to me that it's straightforward with your current structure, but ideally we'd have a single DMS362: Pass, and not three of them.
stscieisenhamer
force-pushed
the
rcaltesting_nojmp
branch
2 times, most recently
from
9644f8d to
a9d2c78
Compare
|
Something tells me there is a name collision however. Too many of the keywords are different, in particular the step completion ones. The truth appears to have completed all steps when the input/result should only have completed dqinit and rampfit. |
stscieisenhamer
force-pushed
the
rcaltesting_nojmp
branch
from
602eaf9 to
9372aab
Compare
|
We have a successful regression! Once CI is finished after the next rebase, this should be good to go. |
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
stscieisenhamer
force-pushed
the
rcaltesting_nojmp
branch
from
271bde5 to
2902803
Compare
Resolves RCAL-661
Resolves RCAL-667
Resolves RCAL-654
Resolves RCAL-656
This PR implements the SOC test requirements for being able to calibrate uneven ramps in both the
RampFitStepandExposurePipelineNote that RCAL_667 was mostly already done in the test
test_wfi_pipeline.pymodule; the requirement message was simply updated. An additional check onImageModelwas added.Note: Currently draft awaiting official test data.
Checklist
CHANGES.rstunder the corresponding subsectionupdated relevant documentation