desihub / desiutil

General DESI utilities, shell scripts, desiInstall, etc.
BSD 3-Clause "New" or "Revised" License
3 stars 9 forks source link

Add radec_to_desiname function #203

Closed akremin closed 11 months ago

akremin commented 12 months ago

This pull request adds a new function, desiutils.names.radec_to_desiname(target_ra, target_dec) that provides the official DESI name for an object at a given sky location. It takes a scalar or array of DESI TARGET_RA's and TARGET_DEC's and returns the new DESINAME's.

The prescribed name is:

image

This is precise to 4 (truncated) decimal places in degrees and is therefore not unique for targets closer than ~0.36”. The J isn't strictly correct but specifying nothing is inferred as B1950, which is worse.

This naming convention does NOT guarantee uniqueness due to closely separated targets within DESI. However, increased precision in the name would be misleading because the fiber is ~1.5" in diameter and light from all sources in that vicinity is collected in an observation.

Investigating the amount of naming collisions in the DESI Y1 Iron catalog:

The vast majority of these naming "collisions" are due to "secondary" targets provided by the collaboration and not within the DESI main survey samples. Restricting to RELEASE >= 1000 removes these secondary targets:

Because early SV1 data included proper motion corrections to their TARGET_RA and TARGET_DEC, a small number (6,588 targets out of 22 million) of TARGETID's have at least two DESINAME's depending on which version of TARGET_RA and TARGET_DEC is used for the objects. Our recommendation is to use the RA and DEC from the main survey without proper motion correction.

coveralls commented 12 months ago

Coverage Status

coverage: 76.284% (+0.1%) from 76.165% when pulling 0b4771902faa5af20b4951c55b7b5fa7dd145f9c on desiname into 93aca730d6b92695027103bb4126772afa90fd0d on main.

akremin commented 11 months ago

Thank you for the review. I have made the requested changes, updated the documentation, and added an extra sentence to the function docstring warning about the non-uniqueness of DESI TARGETID -> DESINAME mapping so that the messaging is in as many places as possible.

sbailey commented 11 months ago

For the record, the included unit tests have nice coverage of various cases:

weaverba137 commented 11 months ago

One thing I did notice though: desiutil does not conduct an API completeness test on itself. I'm going to try to add that test to this branch.

weaverba137 commented 11 months ago

API completeness test has been added, so this can be merged anytime.