Fixes test_fitting_with_initial_values[DogBoxLSQFitter] convergence failure on arm64#17958
Conversation
|
Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.
|
There was a problem hiding this comment.
Welcome to Astropy 👋 and congratulations on your first pull request! 🎉
A project member will respond to you as soon as possible; in the meantime, please have a look over the Checklist for Contributed Code and make sure you've addressed as many of the questions there as possible.
If you feel that this pull request has not been responded to in a timely manner, please send a message directly to the development mailing list. If the issue is urgent or sensitive in nature (e.g., a security vulnerability) please send an e-mail directly to the private e-mail feedback@astropy.org.
|
|
||
| # Fit the data using a Gaussian with units | ||
| g_init = models.Gaussian1D(amplitude=1.0 * u.mJy, mean=3 * u.cm, stddev=2 * u.mm) | ||
| g_init = models.Gaussian1D(amplitude=1.0 * u.mJy, mean=4 * u.cm, stddev=3 * u.mm) |
There was a problem hiding this comment.
I think the original values were added by @astrofrog back in #4855 . So pinging him just in case there was a reason why it was set to 3 and 2.
There was a problem hiding this comment.
Any update on this? :)
|
Hi humans 👋 - this pull request hasn't had any new commits for approximately 4 months. I plan to close this in 30 days if the pull request doesn't have any new commits by then. In lieu of a stalled pull request, please consider closing this and open an issue instead if a reminder is needed to revisit in the future. Maintainers may also choose to add keep-open label to keep this PR open but it is discouraged unless absolutely necessary. If this PR still needs to be reviewed, as an author, you can rebase it to reset the clock. If you believe I commented on this pull request incorrectly, please report this here. |
…] failure on arm64 when using an OpenBLAS optimized for ARM64 using -march=native
2dd2ed9 to
0ebefd9
Compare
|
Sorry for the delay! I took the liberty to rebase and remove change log. Hope you don't mind. This will merge when CI passes. Thanks, all. |
…BoxLSQFitter] convergence failure on arm64
…958-on-v7.1.x Backport PR #17958 on branch v7.1.x (Fixes test_fitting_with_initial_values[DogBoxLSQFitter] convergence failure on arm64)
Description
On ARM64, when using an optimized OpenBLAS (i.e. configured with -march=native), the unit test
test_fitting_with_initial_values[DogBoxLSQFitter]fails to converge. This is because due to the optimization, numerical behavior may alter ever-so-slightly, and apparently the initial starting conditions for this fit we're poorly conditioned. See #15650 for a more extensive description of the issue.After doing an extensive search for various initial conditions here, this PR changes the initial conditions to values that appear to be more robust. I myself have build this on a wide range of CPU architectures (zen2, zen3, zen4, skylake, haswell, sapphire_rapids, x86_64/generic, aarch64/generic, neoverse_v1, neoverse_n1) to check if the test now passes on all of them. For each of those builds, the underlying OpenBLAS was optimized with
-march=native(with the exception of the 'generic' builds), build natively on a machine of one of those architectures. This makes me as sure as I can be that the change in initial condition doesn't fix things for ARM, while reintroducing similar problems elsewhere.Fixes #15650