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

Major refactor of survey planning and tile scheduling #91

Merged
merged 112 commits into from Nov 26, 2018
Merged

Conversation

dkirkby
Copy link
Member

@dkirkby dkirkby commented Nov 14, 2018

This version is a major refactoring of the code to simplify the logic for easier maintenance and documentation. There is now a clean separation between survey strategy, afternoon planning, next-tile selection, and exposure-time calculations. The refactored code is also significantly faster (a full simulation runs in ~2 mins instead of ~6 hours).

The default survey strategy is still the old baseline, but a future version will update this to the changes proposed in Barcelona.

The major missing functionality is a per-exposure moon factor: currently, the moon is treated as a constant factor. This gives ~final results for the DARK program, a good-enough approximation for GRAY, and needs updating for BRIGHT in a future version (but the impact of the BRIGHT program on the survey margin should be correct).

@dkirkby
Copy link
Member Author

dkirkby commented Nov 14, 2018

Tests are passing so this is ready to review and merge. An accompanying surveysim PR is imminent.

@sbailey
Copy link
Contributor

sbailey commented Nov 15, 2018

Thanks!

This is substantial enough that I won't be able to effectively review it other than just running it with surveysim and looking at the outputs. When you open the surveysim PR, please make sure the surveysim/doc/tutorial.rst documentation is up-to-date for how to run it with this branch of desisurvey.

Two options:

  • merge both after tests pass and I'll run and make comments post-facto for any smaller updates
  • wait until I run surveysim+desisurvey and make comments on the outputs before merging

I'm fine with either.

@dkirkby
Copy link
Member Author

dkirkby commented Nov 15, 2018

One thing I would still like to fix in this PR is the proliferation of optional tiles_file arguments, since many classes now have them but others don't. I could add the optional arg everywhere but this is still an error prone design since it requires that the same value is passed in everywhere. This is exactly the problem that the Configuration() class is designed to solve, using these lines at the beginning of a script:

config = desisurvey.config.Configuration()
config.tiles_file.set_value(name)

All desisurvey and surveysim code now uses desisurvey.tiles.get_tiles() to access the tiles file,
so we no longer need to worry about calling desisurvey.io.load_tiles() with the correct args in multiple places.

I suspect an earlier motivation for the optional args was to simplify unit testing, but now that the test branch of desimodel has a trimmed tiles file this shouldn't be an issue.

I propose to:

  • Remove all optional tiles_file args.
  • Use --tiles-file consistently as a cmd-line script arg that sets config.tiles_file.

@sbailey: let me know soon if you object since I believe the tiles_file parameters originated with you.

@moustakas
Copy link
Member

Having the tiles_file argument in more than one place definitely bit me when I was using desisurvey/surveysim for tests of SV, so I support the proposed change.

@sbailey
Copy link
Contributor

sbailey commented Nov 15, 2018

@dkirkby your proposed solution sounds good. Thanks.

@dkirkby
Copy link
Member Author

dkirkby commented Nov 15, 2018

Ok, thanks for the quick feedback.

@moustakas Can you point me to the SV tiles file so I can check that it doesn't break anything in this PR?

@moustakas
Copy link
Member

I haven't looked at this since June, so it's possible the data model changed, but this is the file I had previously created:
/global/projecta/projectdirs/desi/datachallenge/svdc-summer2018/bgs-gama-tiles.fits

@dkirkby
Copy link
Member Author

dkirkby commented Nov 16, 2018

@sbailey If you have time to run through the tutorial today as a sanity check that would be helpful, but otherwise I suggest merging and tagging (this and desihub/surveysim#60) now so others can more easily run the tutorial and give feedback.

@sbailey
Copy link
Contributor

sbailey commented Nov 17, 2018

Thanks. I made a comment over on desihub/surveysim#60 because the new exposures_surveysim.fits format broke surveysim.util.add_calibration_exposures.

Mapping your previous list of output files to document to what I saw on disk when I ran this:

  • Ephemerides written by desisurvey.ephem.Ephemerides = ephem_2019-01-01_2025-12-31.fits?
  • Design HAs written by desisurvey.scripts.surveyinit.calculate_initial_plan = surveyinit.fits
  • Afternoon planning state written by desisurvey.plan.Planner.save = planner*.fits files when using the --save-restore option?
  • Tile scheduler state written by desisurvey.scheduler.Scheduler.save = scheduler_*.fits files when using the --save-restore option?

Do the exposures_surveysim.fits and stats_surveysim.fits files correspond to any file that will be generated by real observing?

More generally can you clarify if these file formats are intended to be what would be generated by real operations? e.g. if we wrote some survey progress tracking tools using exposures*.fits, should we expect that to work on real operations? If not, I'd like to write a script that would generate such a file, because it is really handy to have as an input for survey QA.

@dkirkby
Copy link
Member Author

dkirkby commented Nov 17, 2018

Do the exposures_surveysim.fits and stats_surveysim.fits files correspond to any file that will be
generated by real observing?

This is mostly addressed in this comment on the other PR.

Note that these files are disk dumps of python objects (SurveyStatistics, ExposureList) that already have some useful QA methods, so start there before writing new code.

@sbailey
Copy link
Contributor

sbailey commented Nov 22, 2018

Minor, but would be preferable to get into this PR to minimize the number of times we change the output data model:

  • planner_*.fits files appear to have one row per tile that are assumed to be row-matched to the tiles file. Add a TILEID column to be explicit about that.
  • Add an EXTNAME to planner_*.fits

Also see comments in desihub/surveysim#60, which require coordination with this PR (e.g. overriding the first and last day of the ephemeris calculation in surveyinit).

@dkirkby
Copy link
Member Author

dkirkby commented Nov 26, 2018

@sbailey I have implemented your suggestions above with the latest commits.

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

3 participants