desihub / desisurvey

Code for desi survey planning and implementation
BSD 3-Clause "New" or "Revised" License
2 stars 7 forks source link

Major refactor of survey planning and tile scheduling #91

Closed dkirkby closed 6 years ago

dkirkby commented 6 years ago

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 commented 6 years ago

The files owned by desisurvey that probably need documented data models are:

dkirkby commented 6 years ago

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

sbailey commented 6 years ago

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:

I'm fine with either.

dkirkby commented 6 years ago

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:

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

moustakas commented 6 years ago

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 commented 6 years ago

@dkirkby your proposed solution sounds good. Thanks.

dkirkby commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

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:

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 commented 6 years ago

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 commented 6 years ago

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

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 commented 6 years ago

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