Adjust to numpy dev scalar format by mhvk · Pull Request #15065 · astropy/astropy

mhvk · 2023-07-12T19:53:46Z

DEV NOTE: Do not merge until numpy 2.0 is actually out! ~~Do not merge until this is merged and available in numpy nightly wheel (when we start to see things fail in devdeps)~~

Revert TST: Use legacy numpy 1.25 print #15096 if it got in first. (EDIT: it should get in first; this one is for when 2.0 is actually out)
Decide for scalar quantity.value whether it should not just return an array scalar (see below) -> made this its own issue: Maybe: Let Quantity.value return an array scalar instead of a numpy scalar #16508
Check whether we may want similar adjustments elsewhere (maybe as follow-up; e.g., Structure for structure units output) - but this should not be part of this PR, so punted to Post-numpy-2.0 changes in repr/str #15793.
Decide whether we are happy for the docs to pass tests only on numpy >=2.0. If not, we have to update doctest-plus to ignore np.float64, etc., in comparing numerical values. 2023-Dec: seems that we are heading to a docs-on-numpy>=2.0 only state

This is a start to adjusting our tests for the change in how scalars are formatted in NEP 51 - see numpy/numpy#22449

~~Note that it does not adjust tests of repr and the documentation yet!~~

Posting this in part to show that outside of repr this is not a big deal.

In the documentation, one often will have things like

>>> q = 5 * u.m
>>> q.value
5.0  -> np.float64(5.0)

This brings up a more general question of whether we should use this opportunity of the repr changing to just return an array scalar (i.e., it would be np.array(5.0) instead). EDIT: this is better discussed elsewhere - see #16508.

Fix #15095
Fix #16491

By checking this box, the PR author has requested that maintainers do NOT use the "Squash and Merge" button. Maintainers should respect this when possible; however, the final decision is at the discretion of the maintainer that merges the PR.

github-actions · 2023-07-12T19:54:29Z

github-actions · 2023-07-12T19:54:35Z

👋 Thank you for your draft pull request! Do you know that you can use [ci skip] or [skip ci] in your commit messages to skip running continuous integration tests until you are ready?

pllim · 2023-07-12T21:33:36Z

The diff looks much simpler than I anticipated. Thanks! 🤞

mhvk · 2023-07-12T23:59:05Z

Well, the main diff from the docs is not yet in there...

pllim · 2023-07-26T17:56:04Z

If #15096 goes in first, that PR also has to be reverted here. FYI.

mhvk · 2023-07-26T19:20:27Z

I just rebased and force-pushed to get a fresh test run. If -dev passes, we probably should run extra CI as well, just in case.

p.s. Very pressed for time right now, so if this doesn't converge easily, happy with #15096 as a stop-gap measure...

pllim · 2023-07-26T19:35:51Z

Alas... still 95 failed (though 3 of them are from modeling and unrelated).

pllim · 2023-07-26T19:36:34Z

Looks like we need version check in some places?

ImportError: cannot import name 'get_formatter' from 'numpy.core.arrayprint'

mhvk · 2023-07-26T20:45:51Z

Ah, right, that changed, we didn't make the new get_formatter in the end! Can be a quick change (hopefully)...

pllim · 2023-07-26T21:15:01Z

There are 51 failures now (3 unrelated, the rest doctest output comparison failures). I actually don't like the new look in our own doc test output... especially when you print out the WCS... it looks jarring and adds noise to the actual data I am interested in (say, what is the value of CVAL1, without also have to see it is np.float because I already know that).

mhvk · 2023-08-11T16:20:24Z

The problem with this PR is that all doctests will fail on older numpy. In principle, we can just not run the tests on older versions, but that makes sense only once 2.0 is actually out. I think the logical route is to go with #15096 first, so that things work again, and then do this one once 2.0 is released.

pllim · 2023-08-11T16:30:22Z

In that case might even want to wait for numpy 2.1 for a chance for users to catch up? Or is that too long?

mhvk · 2024-05-28T14:02:18Z

@mhvk in Stingray, we've preferred changing all the problematic doctests to asserts. I know that our approach has other problems, but how stable and platform-independent is relying on np.int64 or similar?

I think in the end we are stuck with fixing up after numpy changes here -- which thankfully doesn't happen too often.

In terms of approach, to me, it is important that the documentation actually looks like code one would run, which makes me very hesitant to change just showing output to assert statements. (Of course, I also rather not go again through the rather large hassle of changing all those doctests!)

But if there are more such changes, perhaps we should consider more seriously making the doctest output checks more "sloppy" (like with FLOAT_CMP).

pllim · 2024-05-28T14:10:37Z

-    "docs/io/fits/index.rst = numpy<1.25",  # NUMPY_LT_1_25 (Issue 14545)
-    "docs/io/fits/usage/image.rst = numpy<1.25",  # NUMPY_LT_1_25 (Issue 14545)
-    "docs/io/fits/usage/unfamiliar.rst = numpy<1.25",  # NUMPY_LT_1_25 (Issue 14545)
+    "docs/* = numpy>=2.0",   # not NUMPY_LT_2_0 (PR 15065)


With this comparison, I think even devdeps won't pick them up, right, because dev is less than stable? How do we know they are actually passing in CI?

In my first run today, some docs tests failed, so I know it works... See also https://github.com/astropy/astropy/actions/runs/9269807248/job/25501391077?pr=15065#step:7:1464, https://github.com/astropy/astropy/actions/runs/9268687018/job/25497881760?pr=15065#step:10:29371. That makes sense, since those are 2.1.dev

However, you are right that the tests do not run on the 2.0rc (i.e., the prereleases daily cron job). If you think it is worth adjusting this accordingly, then I can do that (my sense is to let it be, given that numpy will be out shortly...)

OK then the comment to wait for numpy 2.0 still stands, and we'll just not have to merge huge refactoring PRs until then, to ensure rebasing this won't be painful. 🤞

Because we don't require devdeps to pass to allow merge, and this PR basically is saying that we need devdeps to be green because doctest would only run there until numpy 2 is released.

OK, maybe that's an argument to change the requirements - I don't really want to have to think about this again...

I say let's do what we can to merge it now if possible.

So, is your proposal to change it to numpy>=2.0.dev and then we temporarily make devdeps passing required for merging?

OK, I changed the requirement to 2.0.0rc1, so we get a real failure if something changes.

mhvk · 2024-05-28T16:54:50Z

Hmm, that's annoying, the fix for boost didn't actually work - see failure in daily cron. Turns out #16431 had not caught all occurrences...

@larrybradley - I redid your fix from #16431 in a different way, which makes everything pass. Could you check the last commit, ~~840f022~~ ~~22858d4~~ 4b78dd6?

mhvk · 2024-05-28T17:41:22Z

@pllim - the pre-release test in daily cron now indeed includes the doc tests (and passes), so we will get warnings in the unlikely case there is a change to the documentation between now and the official release.

larrybradley · 2024-05-28T18:37:20Z

@mhvk For sigma_clip, cenfunc and stdfunc can also be input as user-defined callable functions. Do we care if such functions return Python floats? That's why in #16431 I converted all return values from cenfunc and stdfunc into np.float.

mhvk · 2024-05-28T18:48:09Z

@larrybradley - ah, I see. At some level, no, we do not care - ducktyping is good python practice, after all. And really, even here the only reason we found this is because the doctests cannot both pass with and without bottleneck otherwise. Though of course it is nice if the types astropy itself produces do not depend on what package one happens to have installed!

Anyway, I think I'd leave it as it is now...

pllim · 2024-05-28T19:05:50Z

Re: #15065 (comment)

Does this mean the Daily Cron job has to be run now for every PR until numpy 2 is out? 🤔

mhvk · 2024-05-28T19:33:55Z

Re: #15065 (comment)

Does this mean the Daily Cron job has to be run now for every PR until numpy 2 is out? 🤔

That would be one solution, but so few PRs touch the documentation that I think we could just do without...

Obviously, another option is still just to wait for the actual release.

pllim · 2024-05-29T14:05:34Z

We could temporarily add a predeps job to the regular CI as part of this PR, and then make a reminder issue to remove it after. I think we are still a good 3-4 weeks until numpy 2 is released. Not sure if we need remote data though...

astropy/.github/workflows/ci_cron_daily.yml

Lines 48 to 53 in 7202529

    
           - name: Python 3.12 with all optional dependencies and pre-releases 
        
             os: ubuntu-latest 
        
             python: '3.12' 
        
             toxenv: py312-test-alldeps-predeps 
        
             toxargs: -v 
        
             toxposargs: --remote-data=any

mhvk · 2024-05-29T14:49:36Z

How about I change the double run to use predeps? Maybe not even temporarily: it seems somewhat sensible in general to have a predeps run on our important dependencies.

astropy/.github/workflows/ci_workflows.yml

Lines 93 to 94 in 7202529

    
                   - name: Python 3.10 Double test (Run tests twice) 
        
                     linux: py310-test-double

EDIT: possibly make that python 312 as well?

pllim · 2024-05-29T14:59:48Z

Double run uses the test runner, it seems. I remember the test runner not very good with doctest (#8153) so not a good candidate.

astropy/tox.ini

Line 120 in 6f217f4

    
           double: python -c 'import sys; from astropy import test; test(); sys.exit(test())'

mhvk · 2024-05-29T15:38:42Z

OK, I stuck it on the first run with python 3.12 and minimal dependencies.

mhvk · 2024-05-29T15:39:10Z

Removed Extra CI since we just checked that worked.

pllim · 2024-05-29T15:44:22Z

+        # docs.  Fine to remove once numpy 2.0 is released (if problematic).
        - name: Python 3.12 with minimal dependencies and full coverage
-          linux: py312-test-cov
+          linux: py312-test-cov-predeps


Maybe this is okay for a few weeks. It is good to have doctest in coverage.

pllim

Thanks! My goal is to retire before Numpy 3 or Python 4 comes out, whichever first.

pllim · 2024-05-29T17:30:39Z

Thank you so much for going through this pain, @mhvk ! I opened #16512 as a follow-up.

mhvk · 2024-05-29T17:51:41Z

Thanks!!

mhvk added the numpy-dev label Jul 12, 2023

mhvk added this to the v6.0 milestone Jul 12, 2023

github-actions Bot added io.votable table io.fits units utils io.misc utils.masked labels Jul 12, 2023

mhvk mentioned this pull request Jul 12, 2023

ENH: Update scalar representations as per NEP 51 numpy/numpy#22449

Merged

3 of 4 tasks

This was referenced Jul 26, 2023

TST: numpy 2.0 float repr incompatibility in tests astropy/pyvo#464

Closed

TST: Use legacy numpy 1.25 print #15096

Merged

mhvk force-pushed the adjust-to-numpy-dev-scalar-format branch from 28e80f7 to cf64e6c Compare July 26, 2023 19:16

braingram mentioned this pull request Jul 27, 2023

convert numpy scalars to python types before yaml encoding asdf-format/asdf#1605

Merged

eerovaher mentioned this pull request Jul 31, 2023

Cosmo param dataclass #14874

Merged

pllim mentioned this pull request Aug 1, 2023

TST: Handle new numpy 2.0.dev string representation scientific-python/pytest-doctestplus#210

Open

1 task

mhvk force-pushed the adjust-to-numpy-dev-scalar-format branch from ffffe25 to a6c39c6 Compare August 11, 2023 14:31

pllim reviewed Aug 11, 2023

View reviewed changes

Comment thread astropy/units/structured.py Resolved

pllim reviewed May 28, 2024

View reviewed changes

Require numpy>=2.0.0rc1, so tests run on release candidate

a723956

Ensure bottleneck result types are consistent with numpy

4b78dd6

mhvk force-pushed the adjust-to-numpy-dev-scalar-format branch from 22858d4 to 4b78dd6 Compare May 28, 2024 17:03

mhvk removed the Extra CI Run cron CI as part of PR label May 29, 2024

Temporarily use -predeps on one of the regular CI runs

24c6c60

mhvk force-pushed the adjust-to-numpy-dev-scalar-format branch from 7d387fb to 24c6c60 Compare May 29, 2024 15:39

pllim reviewed May 29, 2024

View reviewed changes

pllim approved these changes May 29, 2024

View reviewed changes

pllim enabled auto-merge May 29, 2024 17:00

pllim merged commit 42b96fe into astropy:main May 29, 2024

pllim mentioned this pull request May 29, 2024

TST: Remove predeps from "Python 3.12 with minimal dependencies and full coverage" #16512

Closed

mhvk deleted the adjust-to-numpy-dev-scalar-format branch May 29, 2024 17:51

This was referenced Jun 16, 2024

TST: use numpy 2.0 (stable) in py312-test-cov tox env #16572

Closed

DOC: (v6.1.x) pin numpy<2.0 for consistency with astropy 6.1.0 #16582

Merged

dhomeier mentioned this pull request Jun 26, 2024

TST: a failing doctest on MacOS + numpy 2 astropy/specutils#1146

Closed

Conversation

mhvk commented Jul 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Jul 12, 2023

Uh oh!

github-actions Bot commented Jul 12, 2023

Uh oh!

pllim commented Jul 12, 2023

Uh oh!

mhvk commented Jul 12, 2023

Uh oh!

pllim commented Jul 26, 2023

Uh oh!

mhvk commented Jul 26, 2023

Uh oh!

pllim commented Jul 26, 2023

Uh oh!

pllim commented Jul 26, 2023

Uh oh!

mhvk commented Jul 26, 2023

Uh oh!

pllim commented Jul 26, 2023

Uh oh!

mhvk commented Aug 11, 2023

Uh oh!

pllim commented Aug 11, 2023

Uh oh!

Uh oh!

mhvk commented May 28, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhvk commented May 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhvk commented May 28, 2024

Uh oh!

larrybradley commented May 28, 2024

Uh oh!

mhvk commented May 28, 2024

Uh oh!

pllim commented May 28, 2024

Uh oh!

mhvk commented May 28, 2024

Uh oh!

pllim commented May 29, 2024

Uh oh!

mhvk commented May 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pllim commented May 29, 2024

Uh oh!

mhvk commented May 29, 2024

Uh oh!

mhvk commented May 29, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pllim left a comment

Choose a reason for hiding this comment

Uh oh!

pllim commented May 29, 2024

Uh oh!

mhvk commented May 29, 2024

Uh oh!

mhvk commented Jul 12, 2023 •

edited

Loading

mhvk commented May 28, 2024 •

edited

Loading

mhvk commented May 29, 2024 •

edited

Loading