desihub / desitarget

DESI Targeting
BSD 3-Clause "New" or "Revised" License
19 stars 23 forks source link

Prepare the Main Survey cuts and bit-masks #718

Closed geordie666 closed 3 years ago

geordie666 commented 3 years ago

This PR is focused on updating the "main" cuts and bit-masks in preparation for going on-sky with the Main Survey. It:

geordie666 commented 3 years ago

@sbailey: This PR will likely break integration tests with the mock targets. But, the main reason is that the BGS, ELG and LRG cuts have evolved significantly through sv1, sv2 and sv3. I therefore think that the correct approach is to update the mocks to better reflect the new choices for the real targets.

But, targeting is likely to continue to (rapidly) evolve (and be finalized) in the next week or two. So, my recommendation would be to turn off the integration of the mock targets and real targets for a few weeks. Once the Main Survey cuts are final, I can jury-rig any special logic to integrate the real targets with the mocks. Or, better yet, we might be able to convince @moustakas to spend a couple of hours updating the mocks to make reasonable choices that better approximate the real targets.

Anyone who needs to work with the mocks in the mean-time could just use version 0.57.0 of desitarget.

coveralls commented 3 years ago

Coverage Status

Coverage increased (+0.5%) to 59.393% when pulling 9ec938508167b2da9ddb1d9a855c09f5692ab62f on ADM-main-prep into 2adf2dd1d560c6d3bbc11dc8fb2277c199f05246 on master.

geordie666 commented 3 years ago

I'm finally done with this branch. But, I'll leave @sbailey some time to acknowledge that this branch will break integration with the mock targets before I merge.

sbailey commented 3 years ago

@geordie666 I acknowledge that this will likely break integration with the mocks, and I agree with the suggestion to wait a few weeks for the main survey targeting to settle, and then fix the mocks.

The other thing this could break is fiberassign memory by having 4x more sky targets to be shuffling around. That should be ok for assigning individual tiles, but could be problematic for full-survey runs where we (e.g. @forero) are already having memory trouble. I think desihub/fiberassign#335 is the better long term solution (completely deprecating dr9-based pre-calculated sky catalog), but that requires non-trivial development work. So -- I'm not requesting a change here about the sky density, but I am flagging for our collective awareness in case fiberassign memory becomes a worse problem, and we may need to be prepared to back off on sky target density. @araichoor .

FWIW, the original plan for under-dense sky targets when we were leisurely running fiberassign monthly, was to augment the sky target density only around unassigned fibers, and then rerun fiberassign. That doesn't work for fiberassign-on-the-fly (or fiberassign-in-a-rush) and desihub/fiberassign#335 is the better solution.

geordie666 commented 3 years ago

@sbailey: Thanks for your comments. I'm happy to revert the 4x larger density for sky fibers (it's an easy change), and we can revisit that point if needed? Thoughts?

sbailey commented 3 years ago

@geordie666 if you increased the density in response to a specific request from the survey ops team, ok. If you were just being proactive with trying to get us more sky fiber flexibility, let's test that separately and keep the default unchanged for now.

araichoor commented 3 years ago

for info, about the sky target density and full footprint fiber assignment: with the latest version of fiberassign.bin.desi_fa_smallrun (https://github.com/desihub/fiberassign/pull/333), this may not be an issue.

I proceed per pass (and separating NGC/SGC), with running all tiles from a given pass in parallel, as those don t overlap. The sky targets are read from the desitarget catalogs for each tile separately, using desitarget.io.read_targets_in_tiles - and deleted after having run the fiber assignment for that tile. So, one doesn t need to read an extra-large sky target catalog for the whole footprint at once in the beginning.

With simply requesting two interactive nodes, the dark 7-pass NGC (SGC) runs in 3h (1.5h).

Once the 4x density sky target catalogs are available, I can give a try and see; but I expect that, if I face memory issue, I ll just have to reduce the number of parallel tiles being processed, hence making the run longer.

moustakas commented 3 years ago

Just catching up on all this activity. Yes, I will fix any issues with how the mocks interface with desitarget after things have settled down.