desihub / fiberassign

Fiber assignment code for DESI
BSD 3-Clause "New" or "Revised" License
7 stars 8 forks source link

change breadth first to depth first assignment; check LyA #196

Open sbailey opened 5 years ago

sbailey commented 5 years ago

Currently fiberassign is "breadth first", i.e. if a fiber covers two science targets that have the same PRIORITY but different NUMOBS_MORE, the target with fewer NUMOBS_MORE will be selected, regardless of SUBPRIORITY. From assign.cpp around lines 1419:

            if (tg.priority > best_priority) {
                // Higher priority target class, always choose.
                test_target = true;
            } else if (tg.priority == best_priority) {
                // Same target class.  Use remaining obs and subpriority
                // to choose.
                if (tg.obs_remain > best_obsremain) {
                    test_target = true;
                } else if ( (tg.obs_remain == best_obsremain)
                            && (tg.subpriority > best_subpriority) ) {
                    test_target = true;
                }
            }

This is what LyA wants, but the opposite of what LRG_2PASS want. Since LyA is highest priority anyway, we believe that switching to depth first would give LRG_2PASS what they need, while still giving LyA all of their observations anyway. i.e. try this and test if there is any impact on how many LyA targets get assigned N=4 times:

            if (tg.priority > best_priority) {
                // Higher priority target class, always choose.
                test_target = true;
            } else if (tg.priority == best_priority) &&
                      (tg.subpriority > best_subpriority) {
                    test_target = true;
                }
            }

If it is a big impact for LyA, then we could consider more complicated options like having targets flagged as whether they are a depth-first target or a breadth-first target, but let's avoid that extra bookkeeping unless we really need it.

tskisner commented 4 years ago

The quoted code in this issue no longer exists as of #258. However, the new code makes it possible to have different target classes (i.e. targets with the same PRIORITY) be assigned either depth-first or breadth-first. This is a small change inside the Target::total_priority() function, but it requires some kind of "signal" (like a bit of DESI_TARGET) to indicate whether the target should be breadth-first or depth-first.

tskisner commented 3 years ago

@sbailey , just checking whether you still want this feature? As mentioned, it would be few-line change to alter the target total priority based on a bit of DESI_TARGET. See for example: https://github.com/desihub/fiberassign/blob/5d00eccd04477b003e841a6902f934108d5459b2/src/targets.cpp#L108