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

Replace numpy's inner1d with an alternative implementation #253

Closed mcara closed 10 months ago

mcara commented 10 months ago

This PR replaces the deprecated numpy.core._umath_tests.inner1d() (which will likely error in numpy>=2) with an alternative implementation.

codecov[bot] commented 10 months ago

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Files Coverage Δ
spherical_geometry/great_circle_arc.py 40.45% <66.66%> (+0.92%) :arrow_up:

:loudspeaker: Thoughts on this report? Let us know!.

mcara commented 10 months ago

I also have question of whether the custom C-extension still holds for cross-architecture or even necessary in this age, but that is pondering for another time.

Yes, the custom extension is needed as it performs computations with quad accuracy.

mcara commented 10 months ago

This path is never tested because it is only used when the custom C-extension is missing. How do we know they are equivalent?

Do you have a suggestion?

pllim commented 10 months ago

Do you have a suggestion?

We could force this flag to pretend the C-extension isn't there in one of the jobs. 🤷

https://github.com/spacetelescope/spherical_geometry/blob/c86e676f9b36990733c46682e614fcbd8e543f6b/spherical_geometry/great_circle_arc.py#L24

Also I did accidentally run that mode back in #243 when the jobs refused to build the C-ext at one point, and I discovered some differences:

https://github.com/spacetelescope/spherical_geometry/blob/c86e676f9b36990733c46682e614fcbd8e543f6b/spherical_geometry/tests/test_union.py#L334-L336

computations with quad accuracy

Is that really necessary though?

mcara commented 10 months ago

Is that really necessary though?

If this question about the unit tests, then YES. If it is about the need to use quad accuracy in general, then the answer is again YES.

I do not want to discuss this any further in this PR. In https://github.com/spacetelescope/spherical_geometry/pull/233 I modified a piece of the algorithm to get extra couple of bits of accuracy. Yes, it is needed, in my opinion. But if you would like to have a broader discussion, feel free to open an issue and tag @mdboom @perrygreenfield @WilliamJamieson and others

You can also disable quad accuracy and run the pipelines on many sets from archive. "Many" is the secret here. double accuracy is enough for many sets but for a significant number of data, it is not.

mcara commented 10 months ago

With no formal approval from @pllim this has been merged.

pllim commented 10 months ago

I don't think you need my approval. 😉

But FWIW LGTM. 👏