spacetelescope / spherical_geometry

A Python package for handling spherical polygons that represent arbitrary regions of the sky
http://spherical-geometry.readthedocs.io/
61 stars 31 forks source link

TST: Option to disable using C-extension for debugging #268

Open pllim opened 4 months ago

pllim commented 4 months ago

Maybe sometimes we want to see if tests behave the same with or without C-extensions.

codecov[bot] commented 4 months ago

Codecov Report

Attention: Patch coverage is 91.66667% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 80.35%. Comparing base (49f8c89) to head (8424bd7).

Files Patch % Lines
spherical_geometry/__init__.py 87.50% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #268 +/- ## ========================================== + Coverage 80.33% 80.35% +0.01% ========================================== Files 5 5 Lines 1017 1023 +6 ========================================== + Hits 817 822 +5 - Misses 200 201 +1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

pllim commented 4 months ago

I don't get it. The env var is set correctly. But the tests are not listening to it as expected.

CI: true
DISABLE_SPHR_GEOM_C_UFUNCS: "false"
pllim commented 4 months ago

OK now the jobs ignoring C-ext sees the errors #265 is trying to fix.

If you want this, please SQUASH AND MERGE.

mcara commented 4 months ago

I think we need this. Thanks! However, I think I need to fix failing Python version of the C code first. So let's wait with merging a little.

pllim commented 3 months ago

I rebased and cleaned things up. I think this is ready, but if #271 gets merged first, then I will have to rebase again.

pllim commented 3 months ago

For the record, another option I considered was introducing astropy.config for this package. It is more Pythonic, cleaner, and allow context manager, etc, but I decided that was way overkill for something that is only used for debugging.

Also see: https://docs.astropy.org/en/latest/config/index.html

mcara commented 3 months ago

Wouldn't it be easier to just not build math_util in setup.py?

pllim commented 3 months ago

just not build

How do you do that conditionally, on-demand?

jhunkeler commented 3 months ago

I think we need this.

I agree. The ability to test the python code and math_util functions without rebuilding the package will let us know immediately when one, the other, or both are failing. Declaring HAS_C_UFUNCS once in __init__.py is a nice because we don't have to track down try/except blocks if we want to change the import behavior in the future.

mcara commented 3 months ago

just not build

How do you do that conditionally, on-demand?

Modify Extension in:

https://github.com/spacetelescope/spherical_geometry/blob/49f8c89b16572ae0b84c9637aa56be715ab841c1/setup.py#L115-L119

depending on the environment variable value

Requires re-building but isn't it what is done anyway for each CI test?

pllim commented 3 months ago

Re: https://github.com/spacetelescope/spherical_geometry/pull/268#issuecomment-2113487913

I don't see how that is simpler than what I am proposing.

pllim commented 3 months ago

Also, what I am proposing here allows you to test with and without C-extension locally without rebuilding, as @jhunkeler said above.

mcara commented 3 months ago

Re: #268 (comment)

I don't see how that is simpler than what I am proposing.

Because it is a single change in a single module.