desihub / desitarget

DESI Targeting
BSD 3-Clause "New" or "Revised" License
18 stars 23 forks source link

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

Closed moustakas closed 2 years ago

moustakas commented 2 years ago

Our desisim.templates code has been broken for some time (see https://github.com/desihub/desisim/issues/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.

geordie666 commented 2 years ago

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

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

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

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-- https://github.com/desihub/desisim/pull/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 commented 2 years ago

OK, I'll take a look.

coveralls commented 2 years ago

Coverage Status

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

moustakas commented 2 years ago

@geordie666 this PR is ready for your review.

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

geordie666 commented 2 years ago

@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.