Skip to content

Conversation

jemorrison
Copy link
Collaborator

@jemorrison jemorrison commented Apr 10, 2025

Helps to resolve JP-3857

This PR updates the spectral leak step for code style changes.

Tasks

Copy link

codecov bot commented Apr 10, 2025

Codecov Report

Attention: Patch coverage is 6.89655% with 27 lines in your changes missing coverage. Please review.

Project coverage is 75.58%. Comparing base (5ea9959) to head (b3bed53).
Report is 664 commits behind head on main.

Files with missing lines Patch % Lines
jwst/spectral_leak/spectral_leak_step.py 7.69% 24 Missing ⚠️
jwst/spectral_leak/spectral_leak.py 0.00% 3 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #9395   +/-   ##
=======================================
  Coverage   75.58%   75.58%           
=======================================
  Files         368      368           
  Lines       36898    36898           
=======================================
  Hits        27890    27890           
  Misses       9008     9008           

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jemorrison
Copy link
Collaborator Author

Copy link
Collaborator

@melanieclarke melanieclarke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! It looks like there are no unit tests for this module, but there are regression tests that run this step, so we should be okay for basic code exercise.

Just one quick suggestion for clarity in the docs.

@jemorrison jemorrison force-pushed the JP-3857_spectral_leak_code_style branch 2 times, most recently from 76f4f43 to a4aa36b Compare April 15, 2025 13:08
Copy link
Contributor

@kmacdonald-stsci kmacdonald-stsci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have some recommendations, but it looks fine.

@melanieclarke
Copy link
Collaborator

@jemorrison - there are two small clean up comments here that still need addressing, then I think we should be ready to merge this one.

@jemorrison
Copy link
Collaborator Author

@melanieclarke Sorry I can not find the cleanup you say that still need addressing ?
I change the input_model one and moved the comments to the before the code that Ken suggested.

@jemorrison jemorrison force-pushed the JP-3857_spectral_leak_code_style branch from fe3b507 to b3bed53 Compare April 17, 2025 16:28
@jemorrison
Copy link
Collaborator Author

@melanieclarke I think I did not commit those changes so that might be the problem

Copy link
Collaborator

@melanieclarke melanieclarke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All good now, thanks for the updates.

@melanieclarke melanieclarke enabled auto-merge April 17, 2025 16:32
@melanieclarke melanieclarke merged commit 60d2fa0 into spacetelescope:main Apr 17, 2025
19 checks passed
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