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

add [grz]fiberflux optional arguments to fix (now-broken) desisim/templates code #786

Merged
merged 7 commits into from Jan 7, 2022

Conversation

moustakas
Copy link
Member

Our desisim.templates code has been broken for some time (see desihub/desisim#553) because desisim lagged behind the changes made to desitarget.

This PR is the minimal set of changes to get desisim.templates working again, although I want to add some additional tests on the desisim side (companion PR is forthcoming) so let's not merge yet.

@moustakas moustakas added the WIP Work in Progress; don't merge yet label Jan 6, 2022
@geordie666
Copy link
Contributor

This all looks fine so far, but I'll hold off on my review until it is now longer a WIP. Don't forget to update the changes docs.

Note that we've been having failures on pull, but not on push, which I don't fully understand. But, if push is successful and tests work on your laptop then I think it's fine to merge without messing around too much.

@sbailey
Copy link
Contributor

sbailey commented Jan 7, 2022

I don't understand the pull vs. push test failure difference, but the test failures look like the same fitsio/numpy compatibility issue that was fixed in desihub/desispec#1566. That took a bit of installation-fu from @weaverba137 resulting in https://github.com/desihub/desispec/blob/5859c624b14619718fa4a67b152bcce193683395/.github/workflows/python-package.yml#L73-L78

python -m pip install --upgrade pip wheel
python -m pip install pytest pytest-cov coveralls
python -m pip install git+https://github.com/desihub/desiutil.git@${DESIUTIL_VERSION}#egg=desiutil
python -m pip install -r requirements.txt
python -m pip cache remove fitsio
python -m pip install --no-deps --force-reinstall --ignore-installed 'fitsio${{ matrix.fitsio-version }}'

@weaverba137
Copy link
Member

Let me know if you need assistance in adjusting the test configuration. As far as the difference between push and pull request, I speculate that it could have some subtle difference in the order in which packages are installed, and thus fitsio is built against the ultimately-installed version of numpy in one case but not the other.

@moustakas
Copy link
Member Author

Thank you for the offer, @weaverba137! If you have time I would be grateful for your help updating the tests.

If you have any time to take a look at this PR which is also gloriously failing tests, I'd be grateful--
desihub/desisim#556

desisim has been broken for a long time but I'm having trouble separating out technical/code failures that I need to fix from testing infrastructure failures. Thank you!

@weaverba137
Copy link
Member

OK, I'll take a look.

@coveralls
Copy link

coveralls commented Jan 7, 2022

Coverage Status

Coverage decreased (-0.02%) to 56.515% when pulling 2111c72 on fiberflux-to-colorcuts into a9ab62a on master.

@moustakas moustakas removed the WIP Work in Progress; don't merge yet label Jan 7, 2022
@moustakas
Copy link
Member Author

@geordie666 this PR is ready for your review.

FYI, I'd like to merge this PR before desihub/desisim#556, which has several other moving pieces (including this one).

@geordie666
Copy link
Contributor

@moustakas: I think this is fine to merge. You haven't changed anything else in cuts.py or test_cuts.py since I looked at this yesterday (there are only new kwargs in cuts.py so everything remains backwards compatible). Ben's changes are manifestly OK as all of the unit tests now pass (thanks, Ben).

So, as I said, feel free to merge this to make further progress in desisim.

@moustakas moustakas merged commit 2c4b585 into master Jan 7, 2022
@moustakas moustakas deleted the fiberflux-to-colorcuts branch January 7, 2022 23:02
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

5 participants