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

Change fiber_assignment_order to reflect new pass ordering #111

Merged
merged 2 commits into from Apr 7, 2020

Conversation

schlafly
Copy link
Contributor

@schlafly schlafly commented Apr 1, 2020

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.

…le; remove fiber_assignment_order constraints from BGS.
@schlafly
Copy link
Contributor Author

schlafly commented Apr 1, 2020

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
Copy link
Member

dkirkby commented Apr 1, 2020

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
Copy link
Contributor Author

schlafly commented Apr 1, 2020

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
Copy link
Contributor

sbailey commented Apr 2, 2020

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
Copy link
Contributor

sbailey commented Apr 6, 2020

@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
Copy link
Contributor Author

schlafly commented Apr 6, 2020

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 sbailey marked this pull request as ready for review April 7, 2020 18:42
@sbailey
Copy link
Contributor

sbailey commented Apr 7, 2020

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.

@sbailey sbailey merged commit ebeb41b into master Apr 7, 2020
@sbailey sbailey deleted the new-tiles-config branch April 7, 2020 18:47
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