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

support findfile camera+spectrograph as b,r,z + 0-0 instead of b0,r1..z9 #780

Merged
merged 3 commits into from May 31, 2019

Conversation

sbailey
Copy link
Contributor

@sbailey sbailey commented May 21, 2019

This PR fixes #779, to support io.findfile(..., camera='r9') and io.findfile(..., camera='r', spectrograph=9). Previously only the former was supported.

For context, the spectrograph argument was previously only used for stdstars, which span b/r/z and thus don't request a camera.

@sbailey
Copy link
Contributor Author

sbailey commented May 21, 2019

Having submitted this, I am a bit ambivalent about it, and welcome comments. Previously "camera" meant b0, r1 .. z9, and we have used "arm" or "channel" or "band" to refer to just the b,r,z part. This PR supports calling "camera" just b,r,z if also specifying "spectrograph", which could be a convenience, or it could just encourage non-standard nomenclature and cause confusion if we have to start thinking about "in this code does camera mean b,r,z or b0, r1, .. z9"?

Opinions?

@michaelJwilson
Copy link
Contributor

On checking the existence of the path ... you could at least check the existence of the parent dirs? I'd suggest the current format works well if you know what you're doing, but otherwise... For something labelled 'findfile' it seems reasonable to default to something that already exists, as opposed to writing. If it was me, I'd add a write=False flag to check on only parent dirs and otherwise check on the file path itself.

The other point isn't a big one. But asking for spectrograph does seem confusing in this context. I can close the other ticket.

@weaverba137
Copy link
Member

I do not like the using 'camera' to mean 'b,r,z'. We already have plenty of good names for 'b,r,z'.

@sbailey
Copy link
Contributor Author

sbailey commented May 30, 2019

After discussion, reverting this PR to not support camera='b', 'r', 'z' without the spectrograph number. But I did add a small check to print a slightly more informative error message if the user tries camera='b', spectrograph=9. It purposefully does not check for the existence of the file or automatically create the directory. If you want that, use os.path.isfile(filename) and os.makedirs(os.path.dirname(filename), exist_ok=True).

@sbailey
Copy link
Contributor Author

sbailey commented May 30, 2019

The smallest things always unravel...

The tests correctly caught that we sometimes take advantage of wildcards in camera names to create a string to be used by globs for finding multiple files at once. Updated checks to not reject that...

@sbailey sbailey merged commit 4ba509b into master May 31, 2019
@sbailey sbailey deleted the findfile_camera branch May 31, 2019 00:23
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.

Spectrograph number missing from cframe filepath returned by desispec.io.meta.findfile?
3 participants