Skip to content

Conversation

@weaverba137
Copy link
Member

This PR:

@coveralls
Copy link

coveralls commented Nov 7, 2025

Coverage Status

coverage: 41.586% (-0.01%) from 41.599%
when pulling 7f65dc5 on pyproject-etc
into 57ef537 on main.

@geordie666
Copy link
Contributor

@weaverba137: This looks good to me. One comment and one question:

(1) Can you please updated the doc/changes.rst file?
(2) I'm curious why the coverage has changed so drastically?

@weaverba137
Copy link
Member Author

@geordie666 :

  1. Done
  2. When I moved the coverage configuration into setup.cfg, I allowed for coverage tests on directories that had previously been excluded. Is there still an active reason for these files and directories to be excluded (marked with <--):
[run]
source = py/desitarget
omit =
   py/desitarget/_version.py
   py/desitarget/conftest*
   py/desitarget/cython_version*
   py/desitarget/mock/mockmaker.py  <--
   py/desitarget/mock/build.py  <--
   py/desitarget/setup_package*
   py/desitarget/*/setup_package*
   py/desitarget/*/*/setup_package*
   py/desitarget/sandbox/*  <--
   py/desitarget/sphinx/* <--
   py/desitarget/skyutilities/* <--
   py/desitarget/test/*
   py/desitarget/*/test/*
   py/desitarget/*/*/test/*
   py/desitarget/train/* <--

@weaverba137
Copy link
Member Author

I'm seeing these warnings on Python 3.12 and 3.13:

=============================== warnings summary ===============================
py/desitarget/test/test_cmx.py::TestCMX::test_astropy_fits
py/desitarget/test/test_cmx.py::TestCMX::test_astropy_table
py/desitarget/test/test_cmx.py::TestCMX::test_numpy_ndarray
py/desitarget/test/test_cuts.py::TestCuts::test_astropy_fits
py/desitarget/test/test_cuts.py::TestCuts::test_astropy_table
py/desitarget/test/test_cuts.py::TestCuts::test_numpy_ndarray
  /home/runner/work/desitarget/desitarget/py/desitarget/cuts.py:2088: DeprecationWarning: Bitwise inversion '~' on bool is deprecated and will be removed in Python 3.16. This returns the bitwise inversion of the underlying int object and is usually not what you expect from negating a bool. Use the 'not' operator for boolean negation or ~int(x) if you really want the bitwise inversion of the underlying int.
    photsys_south = ~photsys_north

py/desitarget/test/test_cmx.py::TestCMX::test_astropy_table
py/desitarget/test/test_cuts.py::TestCuts::test_astropy_table
  /home/runner/work/desitarget/desitarget/py/desitarget/cuts.py:2195: UserWarning: Warning: converting a masked element to nan.
    refcat = np.array([refcat, ])

@geordie666
Copy link
Contributor

I think these warnings are not malicious: This is testing we can run a single object (one table row) through the cuts, which we never actually do.

But, open a separate ticket and I'll fix the warnings via that ticket.

Although, thinking about it some more, I thought you'd already fixed these exact issues/warnings in PR #838?

To your question about coverage: Everything marked with a <-- is either third party software that we've "frozen" and incorporated, or the mock code that you deprecated in PR #838. I know that it would be ideal to write our own unit tests for third-party software, but I think it's reasonable to omit everything with a <-- from coverage.

@weaverba137
Copy link
Member Author

I'm on vacation for a couple of days, but I will definitely restore the original coverage configuration when O get a chance.

For the warnings, I'll have to investigate the bitwise operator warning further. It is legitimate though. I may have fixed a very similar NumPy warning, but this warning is coming directly from Python itself.

Copy link
Contributor

@sbailey sbailey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to make photutils, joblib, and sklearn optional dependencies to keep the default end-user installation lighter-weight. Otherwise let's try to move this forward since it is a blocking factor for GitHub tests on other packages passing without requiring workarounds. Coverage and new warning messages can be spun off into separate tickets.

setup.cfg Outdated
fitsio
photutils==1.6.0
joblib
sklearn
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should not make photutils, joblib, and sklearn required dependencies for every install since they make it a much heavier-weight installation for end users who just need the mask bits and misc utility functions. joblib and sklearn are only needed for training the QSO random forest (or more to the point, documenting how it was trained). photutils is used for calculating sky positions and randoms, and in the past it has sometimes been a difficult package to install (perhaps as evidenced by pinning the version here). These 3 packages could be moved into an optional section. Following desiutil's setup.cfg, I think this would go under an "all" section in the [options.extras_require] section below. I'd need to experiment to understand if they also need to be added to the "test" and "coverage" sections.

all =
    photutils==1.6.0
    joblib
    sklearn

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is done in the latest commit. I have also restored the coverage configuration, although I restored it for the files and directories that actually exist. For example mock/mockmaker.py does not exist in the latest version.

@weaverba137
Copy link
Member Author

I have no objections if you want to merge now.

@weaverba137
Copy link
Member Author

I think @sbailey's review is addressed in the latest comments, and with the other issues spun off, I think this can be merged.

@weaverba137
Copy link
Member Author

Hold on! I just realized that the installed dependencies in the unit test differ from those in setup.cfg. I need to run some tests to get those in sync.

@weaverba137
Copy link
Member Author

OK, photutils is definitely needed for tests to pass. Do we still make that an optional requirement?

@weaverba137
Copy link
Member Author

Also note, pytz is required if Python < 3.11. That's a bit trickier to express as an optional requirement.

@sbailey
Copy link
Contributor

sbailey commented Nov 13, 2025

After some further consultation with @geordie666 who happens to be in my office, I've updated this branch so that

  • minimal install of desitarget does not include photutils, but it is included in the full installation via pip install .[all] (reason: maintains lightweight install for analysis end-user, but has available full installation for expert usage)
  • if photutils is not installed, tests that need it will skip instead of crash
  • similar test-skip logic applied for tests that need $DESI_SURVEYOPS to get the official full footprint
  • left photutils in .github/workflows/python-package.yml config so that it will be installed and tested as part of GitHub actions.

That last bullet could likely be cleaned up with further updates to python-package.yml to leverage pip install .[all], but I'll punt that to the future.

@sbailey sbailey merged commit 440076d into main Nov 13, 2025
18 checks passed
@sbailey sbailey deleted the pyproject-etc branch November 13, 2025 01:25
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.

pip install of desitarget fails with pip 25.3 Refactor setup.py installation method

4 participants