Pull common parts of skymatch from romancal and jwst into stcal #310
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #310 +/- ##
==========================================
- Coverage 86.98% 85.00% -1.98%
==========================================
Files 57 61 +4
Lines 10272 11367 +1095
==========================================
+ Hits 8935 9663 +728
- Misses 1337 1704 +367 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
c7e296c to
9ab3734
Compare
9ed58df to
cfaf7eb
Compare
2aee80f to
6202d38
Compare
There was a problem hiding this comment.
This looks great, thanks William! I just had a few questions about whether some of the ruff and mypy problems could be fixed instead of ignored.
I also had a few questions about the code itself, but those also exist on jwst main, so they're technically beyond the scope of this PR. But I'm still curious to know the answers to those and I figure now is as good a time as any to ensure the code is written as cleanly as possible
I agree they should be fixed. However, as you said they are beyond the scope of this PR. I think for making it possible to trace the history of changes it makes sense to move the code in as close a state as possible from JWST to stcal and then do a follow on PR making cleanups and addressing the issues uncovered here. |
IMO making the code adhere to the more stringent style rules in stcal remains within the scope. I don't much like ignoring them. I think the downstream testing should assuage any concerns that those changes might break something. I agree that my comment RE the potentially unnecessary use of |
eb4b7d8 to
2e6b1c9
Compare
705f544 to
f41b760
Compare
|
@emolter I believe this is ready now. |
There was a problem hiding this comment.
All my previous questions have been answered now.
I don't think the changes have been so large that it would be helpful for me to go through it again, but let me know if you'd like me to.
So this looks good to me pending fixing the bug that is causing the failing jwst regtests
dbcfe42 to
ba04e8e
Compare
This is from jwst commit 2491b40a25f42bf2c10d242178434fe6c7d75847
ba04e8e to
1801b3a
Compare
|
It would still be useful to have some unit tests developed for this. We have always relied on regression tests (both for HST and JWST) but PRs to this code in the future should be able to use some quick unit tests. It could be done either as part of this PR or as a follow-up PR. |
I think this should be a follow on PR. I looked into moving some of the unit tests from JWST. But at this time all those tests directly involve running the entire skymatch step. I personally don't feel like I can write any useful unit tests for this part of skymatch independent of JWST at this time. |
@emolter I'm dismissing the request for changes since jwst tests are passing now and you approved otherwise.
|
Thanks @nden, that's fine with me. As I said, I'll only look at this again if needed, @WilliamJamieson I see you re-requested my review after @nden dismissed the stale one. Does that mean you'd like me to look again, or would you just like me to hit the Approve button? |
|
General semi-related question: how much work is left to do in order to move all shared WCS-related functions out of JWST/Romancal and into stcal? Last time I checked, there are several functions with same/similar names and/or same functionality but different names that have multiple versions in the various repositories. It occurs to me that we could have perhaps avoided the confusion caused by these regression test failures if everyone was calling the best and most up-to-date versions of these functions from stcal. I'm talking about functions that should/already do live in Even within stcal there are repeats, e.g. a Is this something we want to clean up in the near future? Should I make an issue/ticket about this? Does it already exist? |
If you want to do another full review your welcome, though there hasn't been any substantive changes since I addressed your last one. In any case, in order for me to merge this PR I do need two approving reviews. |
|
@schlafly @ddavis-stsci This is ready to be merged from JWSt point of view. Romancal unit tests are passing and a PR using this code is in the works for Roman.. Please review when available. |
|
This was waiting for a Roman team member to approve - in general, stcal PRs require approvals not just from any two people, but one from jwst and one from Roman. This still needs eyes from the Roman team. We can make this procedure more clear to the branch. |
I have been tracking this for both JWST and RCAL. JWST was the main sticking point with RCAL waiting for this to go in. |
|
Eddie, the Romancal product owner, listed himself and Dave Davis as approved reviewers for stcal PRs. He is free to add you to that list, but I have not heard of an update there and am trying to follow protocol so that we do not merge changes that Romancal is not aware of. |
|
Thanks. I'm happy for this to go in. I'll note that I got confused trying to understand how the "self.sky" feature was supposed to function in the SkyGroups, and where or how the image.backgrounds are used. I'm sure I just missed it, though, and presumably it is duplicating existing code. |
This PR pulls the common parts of skymatch from romancal and jwst into stcal to be shared.
Note, no tests are included because it is well tested by the step tests in both romancal and jwst.
Tasks
docs/pageno-changelog-entry-needed)changes/:echo "changed something" > changes/<PR#>.<changetype>.rst(see below for change types)"git+https://github.com/<fork>/stcal@<branch>")jwstregression testromancalregression testnews fragment change types...
changes/<PR#>.apichange.rst: change to public APIchanges/<PR#>.bugfix.rst: fixes an issuechanges/<PR#>.general.rst: infrastructure or miscellaneous change