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

Add script that summarizes info about an exposure in a specprod #2124

Merged
merged 6 commits into from Oct 17, 2023

Conversation

akremin
Copy link
Member

@akremin akremin commented Oct 6, 2023

This is still a work in progress. I developed this tool to summarize info about an exposure from sources I thought were relevant. It includes showing the raw data location, the preproc and exposures directories, the pipeline exposure_table entry, the summary exposures-SPECPROD.csv entry, the surveyops exposures.ecsv file, and the entry in the workflow dashboard.

@akremin akremin marked this pull request as ready for review October 11, 2023 22:30
@akremin akremin requested a review from sbailey October 11, 2023 22:30
@sbailey
Copy link
Contributor

sbailey commented Oct 12, 2023

Overall looks useful, though it looks like the --tileid option doesn't work. I was hoping that it could be specified instead of --expid, in which case it would list all exposures for that tile on that night (or ideally without even having to specify --night), but the script still requires --expid to be set. And it lets you specify an --expid that is inconsistent with a --tileid without complaining (it doesn't find the redshift data, but it says "Expected? True").

Examples:

desi_exposure_info -n 20231011 -e 200448
--> tileid 10057, which was also observed on expid 200447, ok

desi_exposure_info -n 20231011 -t 10057
--> says -e/--expid is required

desi_exposure_info -n 20231011 -e 200448 -t 1234
--> inconsistent tileid request, but no errors

@akremin
Copy link
Member Author

akremin commented Oct 12, 2023

Right, I had originally wanted to write this to include use cases for looping over exposures for --tileid or even a --night, but those required more effort and if/else structure than I was willing to put in given my schedule at the moment and the fact that I didn't need it for my use case.

The intention of the script as it exists today is to get information about an exposure. Providing tile or night information reduces the amount of effort it takes to locate the exposure, which is the current purpose of those inputs. For now, if they are unknown or uncertain, the recommendation would be to not specify them. (In my first commit I removed the tileid as unnecessary, but that broke things so I've added it back in).

I have thought about the fact that an incorrect --tileid or --night would be accepted without checking, but for a first prototype, I didn't think it was malicious to assume the provided information was correct. Ideally, if it isn't then the user will get less information than they expected, and they would notice and correct it.

If you consider either of these things to be a dealbreaker, I can improve the relevant area before merging.

@akremin
Copy link
Member Author

akremin commented Oct 13, 2023

Thank you for the review, Stephen. For now I've eliminated the --tileid option and have made the --night option more clear. I also improved the night handling to account for instances where the wrong night is specified. Now it will raise an error.

@sbailey sbailey merged commit 159ae2d into main Oct 17, 2023
24 checks passed
@sbailey sbailey deleted the exposure_info branch October 17, 2023 22:40
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.

None yet

2 participants