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

sunpy 5.1.1 + pytest 8 on armel & mips64el error #7550

Closed
mr-c opened this issue Apr 4, 2024 · 11 comments
Closed

sunpy 5.1.1 + pytest 8 on armel & mips64el error #7550

mr-c opened this issue Apr 4, 2024 · 11 comments
Labels
Bug Probably a bug Priority High Rapid action required

Comments

@mr-c
Copy link

mr-c commented Apr 4, 2024

Describe the bug

Building sunpy 5.1.1 with Pytest 8.1.1 + #7429 we still get two errors:

On armel (arm32v5): test_map_arithmetic_operations_raise_exceptions[map-warn_context0] fails, DID NOT WARN. No warnings of type (<class 'RuntimeWarning'>,) were emitted.

https://buildd.debian.org/status/fetch.php?pkg=sunpy&arch=armel&ver=5.1.1-4&stamp=1712238082&raw=0

On mips64el: [doctest] sunpy.coordinates.frames.Helioprojective.assume_spherical_screen:

Differences (unified diff with -expected +actual):
    @@ -2,4 +2,4 @@
         [(   0., 0., 0.99660825), ( 319., 0., 0.99687244),
          ( 638., 0., 0.99778472), ( 957., 0., 1.00103285),
    -     (1276., 0., 1.00125872), (1595., 0., 1.00125872),
    -     (1914., 0., 1.00125872)]>
    +     (1276., 0.,        nan), (1595., 0.,        nan),
    +     (1914., 0.,        nan)]>

https://buildd.debian.org/status/fetch.php?pkg=sunpy&arch=mips64el&ver=5.1.1-4&stamp=1712238826&raw=0

Any guidance on how we shall fix this?

To Reproduce

Here's a dockerfile, build it with --platform=linux/arm/v5 or --platform=linux/mips64le

FROM docker.io/debian:sid-slim

WORKDIR /build

RUN apt-get update && apt-get install -y --no-install-recommends devscripts curl debian-keyring
RUN dget https://deb.debian.org/debian/pool/main/s/sunpy/sunpy_5.1.1-4.dsc
WORKDIR /build/sunpy-5.1.1
RUN apt-get -y build-dep .
RUN dpkg-buildpackage -uc -us -b

For reference, all the extra patches we apply to Sunpy 5.1.1 are at https://salsa.debian.org/debian-astro-team/sunpy/-/tree/master/debian/patches?ref_type=heads

Screenshots

No response

System Details

==============================
sunpy Installation Information
==============================

General
#######
OS: Debian GNU/Linux (trixie/sid, Linux 6.1.0-18-amd64)
Arch: 64bit, ()
sunpy: 5.1.1
Installation path: sunpy.egg-info

Required Dependencies
#####################
astropy: 6.0.1
numpy: 1.26.4
packaging: 24.0
parfive: 2.0.2

Optional Dependencies
#####################
asdf: 3.1.0
asdf-astropy: Missing asdf-astropy>=0.1.1; extra == "asdf" or "docs" or "tests"
beautifulsoup4: 4.12.3
cdflib: Missing cdflib!=0.4.0,!=1.0.0,>=0.3.20; extra == "docs" or "tests" or "timeseries"
dask: 2023.12.1+dfsg
drms: 0.7.1
glymur: Missing glymur!=0.9.5,>=0.9.1; extra == "docs" or "jpeg2000" or "tests"
h5netcdf: 1.3.0
h5py: 3.10.0
lxml: 5.2.1
matplotlib: 3.6.3
mpl-animators: 1.1.1
pandas: 2.1.4+dfsg
python-dateutil: 2.9.0
reproject: 0.13.0
scikit-image: 0.22.0
scipy: 1.11.4
sqlalchemy: 1.4.50
tqdm: 0.0.0
zeep: 4.2.1

Installation method

from source (.tar.gz)

@Cadair
Copy link
Member

Cadair commented Apr 4, 2024

Thanks for the bug report (and testing sunpy on other platforms) we will try and take a look.

@Cadair Cadair added Bug Probably a bug Priority High Rapid action required labels Apr 4, 2024
@ayshih
Copy link
Member

ayshih commented Apr 4, 2024

On mips64el: [doctest] sunpy.coordinates.frames.Helioprojective.assume_spherical_screen:

Differences (unified diff with -expected +actual):
    @@ -2,4 +2,4 @@
         [(   0., 0., 0.99660825), ( 319., 0., 0.99687244),
          ( 638., 0., 0.99778472), ( 957., 0., 1.00103285),
    -     (1276., 0., 1.00125872), (1595., 0., 1.00125872),
    -     (1914., 0., 1.00125872)]>
    +     (1276., 0.,        nan), (1595., 0.,        nan),
    +     (1914., 0.,        nan)]>

https://buildd.debian.org/status/fetch.php?pkg=sunpy&arch=mips64el&ver=5.1.1-4&stamp=1712238826&raw=0

Apparently, on mips64el, numpy.fmin() does not behave as it should, namely ignoring NaNs (see the long discussion in numpy/numpy#23158). I presume that no fix was discovered for the NumPy build on debian (otherwise we wouldn't be having the problem here). We could refactor code to avoid the problematic call in our code:

d = np.fmin(d, dd) if self._spherical_screen['only_off_disk'] else dd

since in our specific case, the NaNs are from taking the square root of a negative number.

@nabobalis
Copy link
Contributor

This looks to be a bug in Python, which was only fixed in python/cpython#104202 which came out in 3.12.

@Cadair
Copy link
Member

Cadair commented Apr 4, 2024

I have to say I don't think it would be worth us working around this for an architecture I very much doubt anyone is using SunPy on 😁

@ayshih
Copy link
Member

ayshih commented Apr 4, 2024

On armel (arm32v5): test_map_arithmetic_operations_raise_exceptions[map-warn_context0] fails, DID NOT WARN. No warnings of type (<class 'RuntimeWarning'>,) were emitted.

https://buildd.debian.org/status/fetch.php?pkg=sunpy&arch=armel&ver=5.1.1-4&stamp=1712238082&raw=0

It looks like dividing by zero on ARM (with software floating point?) might not always raise a NumPy warning. See scikit-image/scikit-image#3335 for a similar-sounding issue. We can easily change the warning catching to be a warning squashing (using numpy.errstate()) since the presence of a NumPy warning is not relevant to the unit test.

@ayshih
Copy link
Member

ayshih commented Apr 4, 2024

Upon further poking, the fact that we are even dividing in that situation is a bug, so #7551 stops that from happening

@mr-c
Copy link
Author

mr-c commented Apr 5, 2024

Apparently, on mips64el, numpy.fmin() does not behave as it should, namely ignoring NaNs (see the long discussion in numpy/numpy#23158). I presume that no fix was discovered for the NumPy build on debian (otherwise we wouldn't be having the problem here).

Indeed, for the silx package in Debian, we skip the problematic test on mips64el https://salsa.debian.org/science-team/silx/-/blob/master/debian/patches/0009-Skip-testing-nanmin-on-float64-arrays-on-mips-el.patch?ref_type=heads

Shall I do the same for sunpy and the sunpy.coordinates.frames.Helioprojective.assume_spherical_screen doctest?

@ayshih
Copy link
Member

ayshih commented Apr 5, 2024

This looks to be a bug in Python, which was only fixed in python/cpython#104202 which came out in 3.12.

That CPython PR changed things, but evidently did not resolve this issue because the debian log shared above has the test failing under Python 3.12.2.

Shall I do the same for sunpy and the sunpy.coordinates.frames.Helioprojective.assume_spherical_screen doctest?

After discussion among the development team, our preference is to simply skip this doctest. Modifying our code would be clunky and/or affect performance for non-mips64el platforms.

@mr-c
Copy link
Author

mr-c commented Apr 5, 2024

Thanks, @ayshih ! Oddly enough, the mips64el issue went away when I rebuilt with #7551 cherry-picked on top

https://buildd.debian.org/status/fetch.php?pkg=sunpy&arch=mips64el&ver=5.1.1-5&stamp=1712345380&raw=0

@mr-c mr-c closed this as completed Apr 5, 2024
@ayshih
Copy link
Member

ayshih commented Apr 5, 2024

Odd indeed!

@Cadair
Copy link
Member

Cadair commented Apr 5, 2024

Thanks @mr-c

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Probably a bug Priority High Rapid action required
Projects
None yet
Development

No branches or pull requests

4 participants