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

update validredshifts.validate() to work on SV observations #2122

Merged
merged 3 commits into from
Sep 25, 2023
Merged

Conversation

moustakas
Copy link
Member

The convenience script validredshifts.validate() implements the nominal criteria to select "good" redshifts; however, it currently only works on main-survey targets. This simple PR enhances the function to work on SV (but not CMX) data.

This script is not used in any part of production so I have not taken the time to write unit tests, but it would be helpful if this could be merged eventually as part of the template work I'm doing.

@sbailey
Copy link
Contributor

sbailey commented Sep 25, 2023

The code looks fine, but I haven't been able to reproduce the failing case on main so I'm not sure I'm evaluating the right thing. e.g. using desispec/main (not this PR branch) to validate sv1 tile 80611:

from desispec.validredshifts import validate
validate('/global/cfs/cdirs/desi/spectro/redux/iron/tiles/cumulative/80611/20201222/redrock-0-80611-thru20201222.fits')

-->
<Table length=500>
GOOD_BGS GOOD_LRG GOOD_ELG GOOD_QSO      TARGETID                 Z            ZWARN COADD_FIBERSTATUS
  bool     bool     bool     bool         int64                float64         int64       int32      
-------- -------- -------- -------- ------------------ ----------------------- ----- -----------------
    True     True     True    False  39628462817808192      0.1495700223052229     0                 0
   False    False    False    False 616089215125422495      1.5405229350632148     1                 0
    True     True     True    False  39628462822001504     0.10297132568175493     0                 0
    True     True     True    False  39628468098437649      0.3301800074160998     0                 0
...

Regarding "it currently only works on main-survey targets" -- does it crash, or give the wrong answer, or only fail on some tiles, or?

@moustakas
Copy link
Member Author

@sbailey thanks for the review. See these lines:
https://github.com/desihub/desispec/pull/2122/files#diff-00224424e1a4095bc1c7f95139478923c6ac2961fc4e33106de8455e262621d0L117-L119

If you set return_target_columns=True you'll see that it only returns the main-survey target columns (DESI_TARGET, BGS_TARGET, etc.).

In this PR I use desitarget.targets.main_cmx_or_sv (@geordie666) to determine which targeting bits to return, which in the case of your example would be SV1_DESI_TARGET, SV1_BGS_TARGET, etc.

@sbailey
Copy link
Contributor

sbailey commented Sep 25, 2023

Clarifying my confusion: The issue isn't a crash, or that desispec.validredshifts.validate itself was returning DESI_TARGET vs. SV1_DESI_TARGET, etc., but rather that internally validate was using the wrong target columns when calculating the return columns like ELG, LRG, QSO, BGS_ANY, etc. and thus getting them wrong (usually False).

Thanks for the update. Merging now.

@sbailey sbailey merged commit 34b6083 into main Sep 25, 2023
24 checks passed
@sbailey sbailey deleted the validate-sv branch September 25, 2023 23:50
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.

2 participants