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

HLA-1111: Updated SVM filenames for WFPC2 #1678

Conversation

s-goldman
Copy link
Collaborator

@s-goldman s-goldman commented Oct 2, 2023

Resolves HLA-1111

Closes #1662

This PR changes SVM filenames of WFPC2 products. The current SVM filenames follow the following structure:

hst_<propid>_<obsetid>_<instr>_<detector>_<filter>_<ipppss>_dr[cz].fits

where previously the <instr> and <detector> were both WFPC2. With these changes, the <detector> is forced to be "pc" if the <instr> is "WFPC2".

@codecov
Copy link

codecov bot commented Oct 2, 2023

Codecov Report

Attention: 10 lines in your changes are missing coverage. Please review.

Files Coverage Δ
drizzlepac/hapsequencer.py 58.13% <ø> (+6.16%) ⬆️
drizzlepac/runastrodriz.py 41.95% <ø> (+6.61%) ⬆️
drizzlepac/align.py 48.69% <0.00%> (-20.66%) ⬇️
drizzlepac/haputils/align_utils.py 81.12% <0.00%> (+4.22%) ⬆️
drizzlepac/haputils/poller_utils.py 52.06% <0.00%> (+3.71%) ⬆️
drizzlepac/haputils/product.py 65.21% <50.00%> (+3.09%) ⬆️
drizzlepac/wfpc2Data.py 10.58% <0.00%> (-24.46%) ⬇️
drizzlepac/haputils/config_utils.py 68.54% <0.00%> (+13.24%) ⬆️

... and 127 files with indirect coverage changes

📢 Thoughts on this report? Let us know!.

@s-goldman s-goldman added the Do Not Merge PR which should not be merged label Oct 2, 2023
@s-goldman
Copy link
Collaborator Author

I also had to change mentions of wfpc2_wfpc2 in filenames and json files to wfpc2_pc for the code to work properly.

@s-goldman s-goldman removed the Do Not Merge PR which should not be merged label Oct 3, 2023
Copy link
Collaborator

@mdlpstsci mdlpstsci left a comment

Choose a reason for hiding this comment

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

@s-goldman Can you please check PR#1473 where Warren changed the names to include ...wfpc2_wfpc2... In his case he was eliminating both the use of wf and pc. While I can see there are some unrelated changes his PR, it is good to check this PR to make sure all the necessary changes have been addressed by this PR. For example, the environment variable needed for SVM catalog generation for WFPC2 has not been updated in hapsequencer.py in this PR.

@s-goldman
Copy link
Collaborator Author

s-goldman commented Oct 5, 2023

@s-goldman Can you please check PR#1473 where Warren changed the names to include ...wfpc2_wfpc2... In his case he was eliminating both the use of wf and pc. While I can see there are some unrelated changes his PR, it is good to check this PR to make sure all the necessary changes have been addressed by this PR. For example, the environment variable needed for SVM catalog generation for WFPC2 has not been updated in hapsequencer.py in this PR.

I've reverted the changes related to WFPC2 from #PR1473 and the code passes the jenkins regression tests.

@s-goldman s-goldman requested a review from mdlpstsci October 5, 2023 19:28
Copy link
Collaborator

@mdlpstsci mdlpstsci left a comment

Choose a reason for hiding this comment

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

Sorry (!) I know this is tedious, but I think we need to keep the nomenclature and file structure consistent across all instruments. It looks like the [svm | mvm]_parameters need to have the subdirectories for WFPC2 renamed to wfpc2/pc/*.json. This will also require changing the contents of the */index files.

@s-goldman
Copy link
Collaborator Author

Sorry (!) I know this is tedious, but I think we need to keep the nomenclature and file structure consistent across all instruments. It looks like the [svm | mvm]_parameters need to have the subdirectories for WFPC2 renamed to wfpc2/pc/*.json. This will also require changing the contents of the */index files.

No worries, I totally agree. Thanks for catching that. I've made the change in the most recent commit.

@s-goldman s-goldman requested a review from mdlpstsci October 9, 2023 15:07
Copy link
Collaborator

@mdlpstsci mdlpstsci left a comment

Choose a reason for hiding this comment

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

The index file contents have to be updated, and then it is ready to go.

Copy link
Collaborator

@mdlpstsci mdlpstsci left a comment

Choose a reason for hiding this comment

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

Lots of nooks and crannies for this ticket!

@s-goldman s-goldman merged commit fb98283 into spacetelescope:master Oct 10, 2023
@s-goldman s-goldman deleted the HLA-1111_updated_hap_svm_filenames_09_28_23 branch October 10, 2023 15:23
mdlpstsci pushed a commit to mdlpstsci/drizzlepac that referenced this pull request Oct 26, 2023
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.

Update WFPC2 HAP filenames
2 participants