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 an option to only read a subset of columns from the HEALPixel-split MTLs #805

Merged
merged 9 commits into from
Sep 18, 2023

Conversation

geordie666
Copy link
Contributor

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.

@coveralls
Copy link

coveralls commented Aug 29, 2023

Coverage Status

coverage: 56.25% (-0.04%) from 56.291% when pulling 967b737 on ADM-cols-mtl-hp into dd8ba1b on main.

@geordie666
Copy link
Contributor Author

geordie666 commented Aug 29, 2023

@jalasker: Before I merge this, could you check that you can indeed successfully use the new columns kwarg in desitarget.io.read_mtl_in_hp() as requested?

@jalasker
Copy link
Contributor

jalasker commented Sep 7, 2023

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.

  1. only evaluate the is_in_hp function if nside != filenside. This avoids the problem entirely if you are reading in using healpixels/pixlist simply as an alias for which MTL file you want to read in.
  2. Check if "columns" contains RA/Dec and if not (2a) add them to the list of columns or (2b) raise the exception before doing any file loading/processing. (EDIT: It looks like you're already doing 2a in read_targets_in_hp: https://github.com/desihub/desitarget/blob/ADM-cols-mtl-hp/py/desitarget/io.py#L3330)
  3. Combine 1 and 2 by only doing the check in 2 if nside != filenside
  4. Store RA and Dec as separate arrays/tables from anything that will be returned and evaluate is_in_hp regardless of the considerations above.
  5. Number 4, but only do that if RA/Dec aren't in columns

@jalasker
Copy link
Contributor

jalasker commented Sep 7, 2023

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.

@geordie666
Copy link
Contributor Author

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 columns keyword. I think this is defensible given that the coordinates are a crucial aspect of working with HEALPixels. The docstring of read_mtl_in_hp() also now documents this behavior.

Please run your check again and let me know if you want further updates.

@jalasker
Copy link
Contributor

Works as expected and ready to merge.

@geordie666 geordie666 merged commit 506e83b into main Sep 18, 2023
12 checks passed
@geordie666 geordie666 deleted the ADM-cols-mtl-hp branch September 18, 2023 17:06
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.

3 participants