search

desihub / desisurvey

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

allow a different mapping between pass number and program name #85

Closed moustakas closed 3 years ago

moustakas commented 6 years ago

This issue came up as a corner case when using a non-standard tile file (as part of the SV data challenge).

As I understand it, the issue actually arises in desimodel.footprint.pass2program, and should maybe be raised there as well, but I think the only practical consequence is here, in desisurvey.

Here in Progress.get_exposures() by calling pass2program we use the nominal tile file to map pass number to program name, which may not be what we want in a SV-like tile file.

No big deal, but in my case this issue didn't cause any problems until much later in the simulations, which took a while to track down.

Specifically, my tile file only had one program name, DARK, for a handful of tiles. Then, here when the program column gets added on-the-fly

output[name.upper()] = astropy.table.Column(program, description='Program name')

that column ends up inheriting an str4 data type. Then, when calibration exposures get added by surveysim.util.add_calibration_exposures, which have a nominal program name of CALIB, the program name for those get clipped to CALI. Consequently, when desisim/bin/wrap-newexp is called, in this line the calibration exposures get ignored. Phew!

So one possible fix is to ensure that the program column in the exposures table has enough bytes, although I'm sure there are other more elegant solutions.

moustakas commented 6 years ago

This issue is also broadly related to #84.

dkirkby commented 6 years ago

This seems like a reasonable request and should be supported in desisurvey.

However, it looks like desimodel.footprint.pass2program and desimodel.footprint.program2pass call desimodel.io.load_tiles() with no args, so ignore the tiles_file config option that desisurvey uses consistently, e.g.

desimodel.io.load_tiles(onlydesi=True, extra=False, tilesfile=config.tiles_file()))
sbailey commented 6 years ago

Let's update desimodel.footprint.pass2program and program2pass to also accept a tiles table or a tilesfile, in which case it would skip the cached answer and recalculate for those tiles instead.

If desisurvey only calls program2pass a few times (e.g. once per program that it finds) that should be fine. If it for some reason needs to call it once per tile, then we'd need to be smarter with the caching so that it doesn't have to read the tiles file 10k times.

dkirkby commented 6 years ago

I think this is fixed now in #91, but does anyone have a specific test to propose before we close the issue?

Note that desisurvey no longer uses desimodel.footprint.pass2program or desimodel.footprint.program2pass since these are now handled by desisurvey.tiles.Tiles (which also provides other precomputed maps to efficiently support artibrary pass numbering). Also, optional tiles_file args have been eliminated from desisurvey and surveysim, since this proved to be error prone to implement and use. Instead, there is a single config tiles_file parameter that specifies the tiles to use consistently across these packages.

schlafly commented 3 years ago

DavidK proposed closing this three years ago, and desisurvey no longer uses the relevant routines, and the no-pass approach means that the original issue has lost some of its meaning. I'm closing this.