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

Optimize for MWS-specific tiles in minisv3 #601

Merged
merged 17 commits into from Apr 21, 2020
Merged

Optimize for MWS-specific tiles in minisv3 #601

merged 17 commits into from Apr 21, 2020

Conversation

apcooper
Copy link
Contributor

@apcooper apcooper commented Mar 30, 2020

I thought I would document work in progress on this as a PR rather than email threads.

It was mentioned that, ideally, fiberassignment files for minisv3 observations should be produced using only reproducible desitarget and fiberassign code, without the manual tweaking that was involved up to now (e.g. Sarah editing out target classes from certain tiles, changing priorities etc.).

MWS is already OK on this for SV1, but I've only just caught up with the equivalent procedure for SV0/minisv. Thanks to Sarah and Adam for their help.

Now my goal is to create sky-ready fiberassign files for the MWS SV0 tiles proposed at the Tucson meeting for observation in miniSV3 (https://desi.lbl.gov/trac/wiki/MilkyWayWG/MiniSV3). MWS also define VERYBRIGHT counterparts for some of those tiles.

Within the constraint of not hacking anything, my goal is to maximize WD and MWS_CLUSTER targets in these fields and fill the bulk of the remaining fibers with SV0_MWS. The VERYBRIGHTtiles should maximize fibers on theMWS_CLUSTER_VERYBRIGHTtargets and fill withBACKUPtargets and/orSV0_MWS`.

I don't think this is possible with the current cmx_targets.yaml. Tiles with obsconditions BRIGHT are going to be dominated by SV0_BGS (which have higher priority than SV0_MWS) and the MINI_SV* classes (which have very permissive obsconditions).

The only way I can see to get what MWS prefers without hacking outside desitarget is to abuse the condition definitions a little in SV0 (assuming we can get away with this, because tiles are still picked manually at the telescope).

That's the point of this PR. It also includes a few other fixes that might be desirable in any case, but those would be easy to cherry-pick if needs be.

Obsconditions and priorities are set assuming that MWS-specific miniSv3 tiles
will be marked POOR (for regular dark and bright MWS observations) and TWILIGHT
(for tiles with very bright stars).
@apcooper
Copy link
Contributor Author

apcooper commented Mar 30, 2020

The idea is as follows. For miniSV3, define all the "MWS" observations as equivalent to obsconditions=POOR and the "verybright" counterparts as equivalent to obsconditions=TWILIGHT18. Then, tweak obsconditons for MWS targets as follows:

SV0_MWS:
    BRIGHT -> BRIGHT|POOR
SV0_MWS_CLUSTER:
    DARK|GRAY -> BRIGHT|POOR|TWILIGHT12|TWILIGHT18
SV0_MWS_CLUSTER_VERYBRIGHT:
    BRIGHT|POOR|TWILIGHT12|TWILIGHT18 -> TWILIGHT12|TWILIGHT18
SV0_WD:
    BRIGHT -> BRIGHT|POOR|TWILIGHT12|TWILIGHT18
BACKUP_BRIGHT and BACKUP_FAINT:
    POOR|TWILIGHT12|TWILIGHT18 -> TWILIGHT12|TWILIGHT18
SV0_STD_BRIGHT:
    BRIGHT -> BRIGHT|POOR|TWILIGHT12|TWILIGHT18

and remove POOR|TWILIGHT12|TWILIGHT18 from the MINI_SV* classes.

With these changes, I don't think there would be any need to adjust priorities except for the small tweak SV0_MWS_CLUSTER < SV0_MWS_CLUSTER_VERYBRIGHT.

@apcooper
Copy link
Contributor Author

For reference, I see the following mix of targets on the 65008 miniSV3 tile Sarah already made for us by hand. First line is skys. You might need to scroll right to see the number and priority columns

NAME                                                                                   NUMBER PRIORITY
[Non-science targets]                                                                  3151          0
STD_GAIA,STD_DITHER,SV0_MWS                                                            1163       5000
SV0_MWS_CLUSTER                                                                         303      10000
SV0_MWS                                                                                 125       5000
STD_GAIA,SV0_STD_FAINT,STD_DITHER,SV0_MWS,STD_FAINT                                     114       5000
STD_GAIA,SV0_STD_FAINT,SV0_STD_BRIGHT,STD_DITHER,SV0_MWS,STD_FAINT,STD_BRIGHT            74       5000
STD_GAIA,SV0_MWS                                                                         36       5000
STD_DITHER,SV0_MWS                                                                        8       5000
SV0_WD                                                                                    8      20000
STD_GAIA,SV0_STD_FAINT,SV0_STD_BRIGHT,SV0_MWS,STD_FAINT,STD_BRIGHT                        6       5000
STD_GAIA,SV0_STD_FAINT,SV0_MWS,STD_FAINT                                                  5       5000
STD_GAIA,STD_DITHER,SV0_WD                                                                2      20000
STD_GAIA,SV0_WD                                                                           1      20000
STD_GAIA,STD_DITHER,SV0_MWS,SV0_QSO,MINI_SV_QSO                                           1       5000
SV0_MWS,SV0_WD                                                                            1      20000
STD_GAIA,STD_DITHER,SV0_MWS,SV0_WD                                                        1      20000
SV0_BGS,SV0_MWS,SV0_QSO,MINI_SV_QSO,MINI_SV_BGS_BRIGHT                                    1       5000

@apcooper
Copy link
Contributor Author

With tweaks in this PR, I get the following (without manual intervention):

NAME                                                                                 NUMBER   PRIORITY
[Non-science targets]                                                                  3024          0
STD_GAIA,STD_DITHER,SV0_MWS                                                            1145       1500
SV0_MWS_CLUSTER                                                                         442       1600
SV0_MWS                                                                                 121       1500
STD_GAIA,SV0_STD_FAINT,STD_DITHER,SV0_MWS,STD_FAINT                                      99       1500
STD_GAIA,SV0_STD_FAINT,SV0_STD_BRIGHT,STD_DITHER,SV0_MWS,STD_FAINT,STD_BRIGHT            72       1500
STD_GAIA,SV0_MWS                                                                         37       1500
BACKUP_FAINT                                                                             15         10
STD_GAIA,SV0_STD_BRIGHT,STD_DITHER,STD_BRIGHT                                            10          0
STD_DITHER,SV0_MWS                                                                        9       1500
SV0_WD                                                                                    7       2998
STD_GAIA,SV0_STD_FAINT,SV0_STD_BRIGHT,SV0_MWS,STD_FAINT,STD_BRIGHT                        5       1500
STD_GAIA,SV0_STD_FAINT,SV0_MWS,STD_FAINT                                                  4       1500
STD_GAIA,STD_DITHER,SV0_WD                                                                2       2998
SV0_BGS,SV0_MWS                                                                           2       1500
STD_GAIA,STD_DITHER,SV0_MWS,SV0_QSO,MINI_SV_QSO                                           2       1500
STD_GAIA,SV0_WD                                                                           1       2998
SV0_MWS,SV0_WD                                                                            1       2998
STD_GAIA,STD_DITHER,SV0_MWS,SV0_WD                                                        1       2998
SV0_BGS,SV0_MWS,SV0_QSO,MINI_SV_QSO,MINI_SV_BGS_BRIGHT                                    1       1500

@apcooper
Copy link
Contributor Author

@geordie666, I would appreciate your input on this desitarget PR for streamlined production of MWS minisv3 tiles. A more detailed explanation is in the github thread for the PR (above, if you're reading this on github already). @Srheft may be interested too.

Main changes:

  • Made changes to CMX target mask that slightly abuse obsconditions to work around MWS/BGS conflicts for MWS CMX tiles.
  • Fixed (?) logic in calc_priorities for MWS CMX targets.
  • Introduced a new target class, MINI_SV_MWS_FAINT. Might want a different name for this?
  • Updated one test in test_priorities, broken by the above

Some basic diagnostics of the outputs from this PR for MWS minisv3 tiles can be found here:
https://portal.nersc.gov/project/desi/users/apcooper/cmx/auto/

You can click through to specific tiles. For example:
https://portal.nersc.gov/project/desi/users/apcooper/cmx/auto/M53_N5053/65008.html

@apcooper apcooper marked this pull request as ready for review April 17, 2020 06:45
Copy link
Contributor

@geordie666 geordie666 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @apcooper. This PR looks good to me and the obsconditions hack is entirely defensible.

I've included some minor requested changes. Most important is to record the new MINI_SV_MWS_FAINT target class on the wiki. If you don't want to address my other suggestion about making a _cmx_calc_priority() function, that's fine, I can deal with that in a later PR.

I ran this branch end-to-end, so I can confirm it works (through the end of the targeting/MTL stage). In the HEALPixel I ran, there were about ~12% new targets that were purely MINI_SV_MWS_FAINT and after running MTL, for MINI_SV_MWS_FAINT targets (without a zcat), PRIORITY was set to the UNOBS priority (5) and NUMOBS_MORE was 1.

py/desitarget/cmx/data/cmx_targetmask.yaml Outdated Show resolved Hide resolved
py/desitarget/targets.py Show resolved Hide resolved
Spins off special case calc_priority() logic for cmx into a separate function
for readability.
Forgot priority and nobs in previous commit
Copy link
Contributor Author

@apcooper apcooper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made the changes you requested, @geordie666. Thanks.

Copy link
Contributor

@geordie666 geordie666 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good, I tested it end-to-end again and everything appears to work as expected.

Feel free to self-merge this whenever you're ready.

@apcooper apcooper merged commit b6b13ae into master Apr 21, 2020
@apcooper apcooper deleted the mws_sv0_fixes branch April 21, 2020 05:40
@apcooper
Copy link
Contributor Author

Merged, thanks for the checks!

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

2 participants