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 radec_to_desiname function #203

Merged
merged 12 commits into from
Nov 29, 2023
Merged

Add radec_to_desiname function #203

merged 12 commits into from
Nov 29, 2023

Conversation

akremin
Copy link
Member

@akremin akremin commented Nov 28, 2023

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:

  • In the iron catalog (including negative targetids):
    • 868,688 desinames have multiple targetids out of 26,659,579. Percentage=3.26%
    • 6,799 targetids have multiple desinames out of 27,547,223. Percentage=0.02%
  • In the iron catalog (excluding negative targetids):
    • 255,897 desinames have multiple targetids out of 22,979,345. Percentage=1.11%
    • 6,750 targetids have multiple desinames out of 23,229,349. Percentage=0.03%
  • In iron cmx and excluding negative targetids:
    • 0 desinames have multiple targetids out of 5,000. Percentage=0.00%
    • 0 targetids have multiple desinames out of 5,000. Percentage=0.00%
  • In iron sv1 and excluding negative targetids:
    • 533 desinames have multiple targetids out of 839,646. Percentage=0.06%
    • 25 targetids have multiple desinames out of 840,170. Percentage=0.00%
  • In iron sv2 and excluding negative targetids:
    • 312 desinames have multiple targetids out of 139,313. Percentage=0.22%
    • 0 targetids have multiple desinames out of 139,625. Percentage=0.00%
  • In iron sv3 and excluding negative targetids:
    • 77,747 desinames have multiple targetids out of 1,284,712. Percentage=6.05%
    • 0 targetids have multiple desinames out of 1,362,475. Percentage=0.00%
  • In iron main and excluding negative targetids:
    • 161,267 desinames have multiple targetids out of 21,085,015. Percentage=0.76%
    • 0 targetids have multiple desinames out of 21,246,830. Percentage=0.00%

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:

  • In Iron for all surveys (TARGETID > 0 AND RELEASE >= 1000):
    • 795 desinames have multiple targetids out of 21,159,555. Percentage=0.00%
    • 6,588 targetids have multiple desinames out of 21,153,765. Percentage=0.03%
  • In Iron for cmx (TARGETID > 0 AND RELEASE >= 1000):
    • 0 desinames have multiple targetids out of 4,451. Percentage=0.00%
    • 0 targetids have multiple desinames out of 4,451. Percentage=0.00%
  • In Iron for sv1 (TARGETID > 0 AND RELEASE >= 1000):
    • 6 desinames have multiple targetids out of 801,171. Percentage=0.00%
    • 25 targetids have multiple desinames out of 801,152. Percentage=0.00%
  • In Iron for sv2 (TARGETID > 0 AND RELEASE >= 1000):
    • 1 desinames have multiple targetids out of 136,411. Percentage=0.00%
    • 0 targetids have multiple desinames out of 136,412. Percentage=0.00%
  • In Iron for sv3 (TARGETID > 0 AND RELEASE >= 1000):
    • 77 desinames have multiple targetids out of 1,236,535. Percentage=0.01%
    • 0 targetids have multiple desinames out of 1,236,614. Percentage=0.00%
  • In Iron for main (TARGETID > 0 AND RELEASE >= 1000):
    • 577 desinames have multiple targetids out of 19,419,142. Percentage=0.00%
    • 0 targetids have multiple desinames out of 19,419,722. Percentage=0.00% (edited)

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.

@akremin akremin requested a review from weaverba137 November 28, 2023 21:51
@coveralls
Copy link

coveralls commented Nov 28, 2023

Coverage Status

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

Copy link
Member

@weaverba137 weaverba137 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor formatting in the top-level docstring, but otherwise looks fine to me.

Please add names to doc/api.rst and update doc/changes.rst.

py/desiutil/names.py Outdated Show resolved Hide resolved
py/desiutil/names.py Outdated Show resolved Hide resolved
@akremin
Copy link
Member Author

akremin commented Nov 29, 2023

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
Copy link
Contributor

sbailey commented Nov 29, 2023

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

  • list vs. array vs. scalar inputs
  • standard is truncation to 4 decimal places, not rounding, including for negative numbers
  • proper handling of pre-zero padding
    That covers all of the cases that I have thought to test.

@weaverba137
Copy link
Member

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
Copy link
Member

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

@akremin akremin merged commit 0f4e08d into main Nov 29, 2023
20 checks passed
@akremin akremin deleted the desiname branch November 29, 2023 22:00
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.

4 participants