Skip to content

Make confSummary_test default #91

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

Merged
merged 1 commit into from
Jul 2, 2024
Merged

Make confSummary_test default #91

merged 1 commit into from
Jul 2, 2024

Conversation

albireox
Copy link
Member

@albireox albireox commented Jul 2, 2024

Replaces the definition for confSummary and confSummaryF with the same one as for confSummary(F)_test (i.e., with the thousands grouping in addition to the hundreds one).

For now we are keeping the definitions for confSummary(F)_test, which are now identical to the ones for confSummary(F).

Copy link
Collaborator

@havok2063 havok2063 left a comment

Choose a reason for hiding this comment

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

Does this change need to also be in the ipl-3.cfg? If so, can you update the paths there as well.

@albireox
Copy link
Member Author

albireox commented Jul 2, 2024

I'm not sure since IPL-3 may have been run using the old directories (although I'm also not sure that matters since at the end of the day the environment variable does not change). @Sean-Morrison had thoughts on this I think.

@Sean-Morrison
Copy link
Contributor

For the BOSS DRP, the confSummary(F)_test paths from sdss_access and tree were used. Therefore maintaining the _test paths is important. As for the other paths, I don't have a strong feeling especially since I think the Apogee DRP (@dnidever) used hard-coded paths.

@dnidever
Copy link
Contributor

dnidever commented Jul 2, 2024

apogee_drp 1.3 for IPL-3 used hardcoded confSummary paths in apload.py
https://github.com/sdss/apogee_drp/blob/4ae4b00f8c4feb7e294455b77a013f7ceb079521/python/apogee_drp/utils/apload.py#L1051

@albireox albireox requested a review from havok2063 July 2, 2024 18:31
@albireox
Copy link
Member Author

albireox commented Jul 2, 2024

@havok2063 @joelbrownstein When you have a change can you double check everything and if it looks fine approve the PR?

@albireox albireox merged commit 3ac5a43 into main Jul 2, 2024
4 checks passed
@albireox albireox deleted the albireox/confSummary branch July 2, 2024 19:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants