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

encode/decode_negative_targetid from ra,dec #724

Merged
merged 4 commits into from May 6, 2021
Merged

Conversation

sbailey
Copy link
Contributor

@sbailey sbailey commented May 5, 2021

This PR provides prototype code for generating a negative TARGETID based upon (ra,dec), unique to within ~1.2 milliarcsec. These are negative to distinguish them from the positive TARGETIDs generated from encode_targetid(objid, brickid, release, ...) from imaging surveys objects.

This PR is motivated by desihub/fiberassign#335 for creating multiple sky targets on-the-fly. Currently when fiberassign creates a sky TARGETID it is based upon TILEID and positioner LOCATION, but this isn't unique if we start creating multiple sky targets per positioner, some of which will overlap other positioners. This PR provides a standardized way of generating these TARGETIDs in desitarget, which I think should "own" the concept of TARGETID rather than putting this code into fiberassign. It may also be useful for other future cases with new programs.

Discuss:

  • basic concept? function name? bike shedding?
  • by using somewhat lower resolution, we could reserve some bits at the top to optionally encode something like program. Useful?
  • it occurs to me that existing tiles have already claimed some negative TARGETIDs based on TILEID and LOCATION, which would conflict with some unlucky ra,dec. Numbering collisions are very unlikely, but we could avoid them by setting a bit that isn't used in the previous method.
  • other ideas and requests?

@geordie666 @dstndstn @tskisner

Note: once we converge on what we want, I'd also add some unit tests.

@coveralls
Copy link

coveralls commented May 5, 2021

Coverage Status

Coverage increased (+0.1%) to 59.303% when pulling 3e609c9 on create_targetid into 05db498 on master.

@geordie666
Copy link
Contributor

A few thoughts. None of which are compulsory to consider or implement:

  • I'd prefer a name for the function like create_negative_targetid or create_spatial_sky_targetid so that the function isn't confused with the actual encode_targetid function. Better still would be encode_negative_targetid or encode_spatial_sky_targetid. You're likely going to tell me that this will break something in fba!
  • What about the corresponding decode_negative_tagetid function? You might not think this will be useful, but I didn't think decode_targetid would be useful and I use it all of the time to determine the provenance of corner cases. You wouldn't believe how often somebody sends me a TARGETID and asks "quickly, what do you know about this target?!". I can imagine it would useful in the case of the -ve TARGETIDs to be able to quickly say, "it's at this RA/Dec, look that up in the viewer," or "it touched these tiles" etc.
  • The encode/decode targetid functions are set up to use information in the yaml file to assign the bits. For consistency, we might want to persist with that formalism.
  • Provided you always only ever use -ve TARGETIDs, I think it will be impossible to conflict with non-sky-fiber TARGETIDs. So, that's great, and please always keep doing that!

@sbailey sbailey changed the title prototype create_targetid from ra,dec encode/decode_negative_targetid from ra,dec May 6, 2021
@sbailey
Copy link
Contributor Author

sbailey commented May 6, 2021

thanks for the comments @geordie666 . Updates:

  • renamed to targetid = encode_negative_targetid(ra,dec,group=1)
  • added ra, dec, group = decode_negative_targetid(targetid)
  • added optional encoding of an integer group 1-15, default 1. These are stored in high bits and ensure that none of these targetids will collide with the previously used negative targetids from fiberassign. Using a group>1 might be useful in the future for non-sky non-LS ra,dec-based negative TARGETIDs. Maybe.
  • added unit tests

Although in principle I agree with your yaml file parameters, in practice I've run out of steam tonight and I'm not sure it really matters here. The number of bits are hardcoded in two functions, and if those ever get out of sync the unit tests will catch that (which I also confirmed).

Summary:

#- Generate a negative TARGETID that is unique to at least 2 milliarcsec
targetid = encode_negative_targetid(ra, dec)

#- Optionally also encode a group number (default is 1)
targetid = encode_negative_targetid(ra, dec, group=2)

#- Get the ra,dec,group back out
ra1, dec1, group1 = decode_negative_targetid(targetid)

@geordie666
Copy link
Contributor

Thanks @sbailey. These changes all look good to me.

@sbailey sbailey merged commit 38413c0 into master May 6, 2021
@sbailey sbailey deleted the create_targetid branch May 6, 2021 17:27
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.

None yet

3 participants