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

remove lib.dms #1433

Merged
merged 3 commits into from
Oct 21, 2024
Merged

remove lib.dms #1433

merged 3 commits into from
Oct 21, 2024

Conversation

braingram
Copy link
Collaborator

@braingram braingram commented Oct 2, 2024

This PR removes romancal.lib.dms. The log function in this sub-module is no longer a DMS requirement and was moved to romancal/regtest/test_ramp_fitting.py.

Regression tests all pass: https://github.com/spacetelescope/RegressionTests/actions/runs/11279841228

Tasks

  • request a review from someone specific, to avoid making the maintainers review every PR
  • add a build milestone, i.e. 24Q4_B15 (use the latest build if not sure)
  • Does this PR change user-facing code / API? (if not, label with no-changelog-entry-needed)
    • write news fragment(s) in changes/: echo "changed something" > changes/<PR#>.<changetype>.rst (see below for change types)
    • update or add relevant tests
    • update relevant docstrings and / or docs/ page
    • start a regression test and include a link to the running job (click here for instructions)
      • Do truth files need to be updated ("okified")?
        • after the reviewer has approved these changes, run okify_regtests to update the truth files
  • if a JIRA ticket exists, make sure it is resolved properly
news fragment change types...
  • changes/<PR#>.general.rst: infrastructure or miscellaneous change
  • changes/<PR#>.docs.rst
  • changes/<PR#>.stpipe.rst
  • changes/<PR#>.associations.rst
  • changes/<PR#>.scripts.rst
  • changes/<PR#>.mosaic_pipeline.rst
  • changes/<PR#>.patch_match.rst

steps

  • changes/<PR#>.dq_init.rst
  • changes/<PR#>.saturation.rst
  • changes/<PR#>.refpix.rst
  • changes/<PR#>.linearity.rst
  • changes/<PR#>.dark_current.rst
  • changes/<PR#>.jump_detection.rst
  • changes/<PR#>.ramp_fitting.rst
  • changes/<PR#>.assign_wcs.rst
  • changes/<PR#>.flatfield.rst
  • changes/<PR#>.photom.rst
  • changes/<PR#>.flux.rst
  • changes/<PR#>.source_detection.rst
  • changes/<PR#>.tweakreg.rst
  • changes/<PR#>.skymatch.rst
  • changes/<PR#>.outlier_detection.rst
  • changes/<PR#>.resample.rst
  • changes/<PR#>.source_catalog.rst

Copy link

codecov bot commented Oct 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.11%. Comparing base (39b1404) to head (4e55afb).
Report is 190 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1433      +/-   ##
==========================================
+ Coverage   78.07%   78.11%   +0.03%     
==========================================
  Files         119      118       -1     
  Lines        7695     7689       -6     
==========================================
- Hits         6008     6006       -2     
+ Misses       1687     1683       -4     
Flag Coverage Δ *Carryforward flag
nightly 62.03% <ø> (ø) Carriedforward from ac98a17

*This pull request uses carry forward flags. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@braingram braingram marked this pull request as ready for review October 2, 2024 21:26
@braingram braingram requested a review from a team as a code owner October 2, 2024 21:26
@schlafly
Copy link
Collaborator

I think I understand this to remove the existing log messages that describe what we're checking when we eventually mark a requirement as passed in favor of just having DMSXYZ: PASS. We have been telling I&T what we're testing when we run these tests. It's unclear to me how much they actually care about seeing those messages but I do think they have value. Do I understand correctly that we no longer see those messages and pass, and only see them on failure?

@braingram
Copy link
Collaborator Author

I think I understand this to remove the existing log messages that describe what we're checking when we eventually mark a requirement as passed in favor of just having DMSXYZ: PASS. We have been telling I&T what we're testing when we run these tests. It's unclear to me how much they actually care about seeing those messages but I do think they have value. Do I understand correctly that we no longer see those messages and pass, and only see them on failure?

Thanks for taking a look. Yeah this PR removes log messages like this:

DMS362 MSG: Testing that result file path has file type "asdf".......PASS

The mapping of tests to DMS requirements is now being handled by the dms_requirent_tests.json:


so from the DMS side of things these logs aren't needed.

With this PR the above log message won't be produced and if I force that check to fail I see the following output from pytest:

FAILED romancal/regtest/test_ramp_fitting.py::test_rampfit_step[spec_full] - AssertionError: Testing that result file path has file type "asdf"

If it's helpful to retain the log messages for romancal developers this PR could be reduced to maybe just an update to the docstring for romancal/lib/dms.py:

"""Functionality required by DMS outside of core functionality of the package
Example: Certain tests are required by DMS to log in a specific way. The
decorator for this is defined in this module.
"""

Let me know if this is preferred.

@schlafly
Copy link
Collaborator

Yeah, I think we want to retain the success messages; we flag to I&T to look for them to signal that we have done something. I hear you that they aren't needed in the sense that the DMS metrics logger is using the json file instead.

@braingram braingram marked this pull request as draft October 10, 2024 17:21
@braingram braingram changed the title remove custom dms requirement logging in ramp fit regtests remove lib.dms Oct 10, 2024
@braingram braingram marked this pull request as ready for review October 10, 2024 19:55
@braingram braingram merged commit 704873b into spacetelescope:main Oct 21, 2024
31 checks passed
@braingram braingram deleted the ramp_dms branch October 21, 2024 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants