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

coadd bitwise OR of targeting bits #2094

Merged
merged 2 commits into from Aug 25, 2023
Merged

coadd bitwise OR of targeting bits #2094

merged 2 commits into from Aug 25, 2023

Conversation

sbailey
Copy link
Contributor

@sbailey sbailey commented Aug 12, 2023

This PR fixes #2077 by coadding the bitwise OR of targeting bits instead of assuming that they are the same on every tile (see #2077 for context).

I added a unit test with toy examples, but would also like to test it on real cases if @stephjuneau @geordie666 or @araichoor could dig up the email/slack threads to post examples.

@geordie666 and/or @stephjuneau, please double check that I got the complete set of column names correct (I had a few iterations of confusion on SV1_DESI_TARGET vs. SV1_TARGET, etc). For consideration: Instead of a specific list of colnames, I could update this to propagate bitwise OR for any column that matches

colname.endswith('_TARGET') and colname != FA_TARGET

thoughts?

@geordie666
Copy link
Contributor

geordie666 commented Aug 14, 2023

You might want to include a specific unit test for SV1_MWS_TARGET. There are three cases where SV1_MWS_TARGET changed, as documented near the top of #2077 (comment).

(also, there are some specific cases quoted in that comment).

@geordie666
Copy link
Contributor

Also, I think you've captured the full set of relevant target names, yes. I would advise against automating this further to:

colname.endswith('_TARGET') and colname != FA_TARGET

in case we update the data model in the future (or for DESI-2, etc.) and introduce a new _TARGET column without realizing the consequences for coaddition.py.

@sbailey
Copy link
Contributor Author

sbailey commented Aug 25, 2023

I expanded the unit tests to also check that SVn_MWS_TARGET and SVn_BGS_TARGET are also bitwise-or propagated.

I checked the results of this branch compared to @stephjuneau's fuji patch file at /global/cfs/cdirs/desi/public/edr/vac/edr/zcat/fuji/v1.0/zall-pix-targeting-patch-edr.fits using code /global/cfs/cdirs/desi/users/sjbailey/debug/pr2094_targetor/compare_patch.py . Three categories:

  1. previously incorrect coadds that are now fixed by this PR
  2. fiberassign files that were incorrect at the time of running fuji (missing SCND bits), and thus the fuji fibermaps have the wrong inputs to coadd, but were fixed prior to iron so there is nothing new to fix for DR1
  3. cases where an observed target had one set of bits, but then later the target gained secondary target bits and was a potential target on a new tile, but was not observed. Since the coadd only knows about observed targets, it only knows about the original set of bits, but the patch file was based upon the bitwise-OR including the potential targets and thus had the superset of bits. In this case I think it is correct for the coadd to only use the bits from the observed targets given as input.

An example case for item 3: TARGETID=39632940065359576 was observed on tile 80690 using 0.47.0 target catalogs as an SV1_DESI_TARGET MWS_ANY, but then later was a potential target on tile 080691 using 0.50.0 target catalogs where it gained the SCND_ANY bit. Since none of the observations had SCND_ANY set, the coadded fibermap won't have it either, but a DB load of that target based upon the potential targets would include entries with SCND_ANY set.

i.e. I think for the purposes of this PR and the coadding code, everything is fine, and any remaining apparent discrepancies due to observed vs. potential targets are a DR1 documentation level thing.

@sbailey sbailey merged commit 573cfad into main Aug 25, 2023
24 checks passed
@sbailey sbailey deleted the target_or branch August 25, 2023 20:48
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.

Coadd the targeting columns with BITWISE_OR
2 participants