desihub / desisurvey

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

Change fiber_assignment_order to reflect new pass ordering #111

Closed schlafly closed 4 years ago

schlafly commented 4 years ago

The new tile file changed the pass ordering. This PR changes the config file to reflect that change. It also removes the fiber_assignment_order constraints from the BGS/MWS surveys, since I do not believe they intend to impose such constraints.

schlafly commented 4 years ago

This apparently still fails a unit test, but I am unable to reproduce at NERSC. i.e.,

source /project/projectdirs/desi/software/desi_environment.sh master
python setup.py test

succeeds.

dkirkby commented 4 years ago

It seems to be a genuine problem in a unit test:

Traceback (most recent call last):
  File "/home/travis/build/desihub/desisurvey/py/desisurvey/test/test_tiles.py", line 45, in test_overlap
    self.assertTrue(np.all(tile_over[DARK2][IN1]))
AssertionError: False is not true

Is python setup.py test actually running the same tests? Can you paste your output?

schlafly commented 4 years ago

source /project/projectdirs/desi/software/desi_environment.sh master python setup.py test

test_overlap (desisurvey.test.test_tiles.TestTiles) ... ok Ran 64 tests in 20.185s OK I'm probably setting things up wrong or shouldn't expect this to work, but this looks fine to me.
sbailey commented 4 years ago

I confirm that this branch passes tests at NERSC when using desimodel/master, but that it fails when using desimodel/0.10.3. The travis tests are based upon desimodel svn data branches/test-0.10, which is a trimmed down version of the desimodel/0.10.x data. Ideally the code and unit tests would work both with the new tiles file in master and the old tiles file in 0.10.x. At NERSC you can swap back and forth with:

source /project/projectdirs/desi/software/desi_environment.sh master
module unload desisurvey
module swap desimodel/0.10.3
[do your desisurvey branch testing]
module swap desimodel/master
[do your desisurvey branch testing]
sbailey commented 4 years ago

@schlafly @dkirkby pinging regarding this PR since I'd like to include it in a new set of tags this week. Ideally it would support both the new and old tile files, not so much because we particularly want the old tile file, but rather because there appear to be lingering assumptions about the pass:program mapping instead of letting that be entirely defined by the tile file itself. That might come back to bite us if/when we add tile definitions for twilight or badconditions programs.

A fallback option is to go ahead and merge this and say that desisurvey after tag X.Y.Z requires desimodel after P.Q.R, and separately deal with the maintainability concerns.

schlafly commented 4 years ago

I'm sure we could get this to work with more development, but that goes beyond the "update the config file" that I had planned. I won't be able to get to this this week.

sbailey commented 4 years ago

I switched this branch to use desimodel svn data branch test-0.12 to get the new tiles file, but oddly Travis tests seem to have become disassociated from this PR. I verified that tests pass with that branch of desimodel.

I'm going to go ahead and merge this. Having desisurvey work with the new tiles file but not the old (this branch) is better than having it work with the old file but not the new (master prior to merging this). Issue #112 will track future updates for flexibility with updating the tile files.