-
Notifications
You must be signed in to change notification settings - Fork 22
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 an option to only read a subset of columns from the HEALPixel-split MTLs #805
Conversation
@jalasker: Before I merge this, could you check that you can indeed successfully use the new |
I tested this function and there is some slightly unexpected (to me) behavior. When you are requesting RA and Dec as columns, the function works properly. However, if you do not provide RA and Dec as columns, there is a step at the end of the function (https://github.com/desihub/desitarget/blob/ADM-cols-mtl-hp/py/desitarget/io.py#L3238) which "restrict the targets to the actual requested HEALPixels" and which requires RA and Dec to be present. The function will raise a ValueError that there is no RA field in the event that RA/Dec are not listed as columns. I'm assuming this line is so you can provide healpixels with a different size than the ones the MTLs are partitioned into natively, since if the nside = 32, it shouldn't be possible for any trimming to change anything. A couple of changes which I think might work here in a rough order based on how intuitive they'd be for me to use combined with what I think would be easiest to implement.
|
One more semi-related comment: the 'columns' argument can now be propagated through "read_targets_in_hp" to "read_mtl_in_hp" when mtl == True. |
Thanks for the careful and thorough review @jalasker. I chose your option (2a) and always add the RA, DEC columns even if they aren't requested in the Please run your check again and let me know if you want further updates. |
Works as expected and ready to merge. |
This PR adds an option in
desitarget.io.read_mtl_in_hp()
to facilitate reading a subset of columns from an MTL rather than the entire ledger. This should address #802.This PR also addresses some deprecation warnings, which should at least partially address #804.