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.

Sign up for GitHub

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

Jump to bottom

TST: Handle flaky scipy 1.13 in OSX ARM64 #16329

Closed
wants to merge 1 commit into from
Closed

TST: Handle flaky scipy 1.13 in OSX ARM64 #16329

wants to merge 1 commit into from

Conversation

pllim
Copy link
Member

@pllim pllim commented Apr 23, 2024

Description

This pull request is to

  • 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.

@pllim pllim added this to the v6.1.1 milestone Apr 23, 2024
@github-actions GitHub Actions
Copy link

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.

  • Do the proposed changes actually accomplish desired goals?
  • Do the proposed changes follow the Astropy coding guidelines?
  • Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
  • Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
  • Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see instructions for rebase and squash.
  • Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the "Extra CI" label. Codestyle issues can be fixed by the bot.
  • Is a change log needed? If yes, did the change log check pass? If no, add the "no-changelog-entry-needed" label. If this is a manual backport, use the "skip-changelog-checks" label unless special changelog handling is necessary.
  • Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
  • At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate "backport-X.Y.x" label(s) before merge.

@github-actions GitHub Actions
Copy link

👋 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
Copy link
Member Author

pllim commented Apr 23, 2024

pre-commit.ci autofix

@pllim pllim force-pushed the tst-scipy-osx-arm64-flaky branch from d5d3ed2 to a898ba3 Compare April 23, 2024 14:40
neutrinoceros
neutrinoceros reviewed Apr 23, 2024
View reviewed changes
astropy/modeling/tests/test_quantities_fitting.py
Comment on lines +116 to +122
# https://github.com/astropy/astropy/issues/16320 (scipy can be flaky in OSX ARM64)
if sys.platform == "darwin":
ctx = np.errstate(invalid="ignore")
else:
ctx = contextlib.nullcontext()
with ctx:
g = fitter(g_init, x, y)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to avoid going with this patch if we can. The behavior isn't flaky, per say: it's deterministic. I'm working on reducing the reproducer outside of astropy so scipy folks can inspect what's going on more easily.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we can go with #16330 instead but I'd rather not have our CI red until scipy has time to fix it. Thanks for the quick feedback!

docs/coordinates/matchsep.rst
@@ -250,7 +251,7 @@ the catalog:
.. doctest-requires:: scipy

>>> matches = catalog[idx]
>>> (matches.separation_3d(c) == d3d).all()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this one is correct, I think

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would wait to see what scipy end up doing if we going to pin in #16330 , just in case if their fix also get rid of the need to update this.

@pllim pllim closed this Apr 23, 2024
@pllim pllim deleted the tst-scipy-osx-arm64-flaky branch April 23, 2024 14:51
@pllim pllim mentioned this pull request Apr 23, 2024
Draft
1 task
@neutrinoceros neutrinoceros mentioned this pull request Apr 24, 2024
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@neutrinoceros neutrinoceros neutrinoceros left review comments

Assignees
No one assigned
Labels
backport-v6.1.x on-merge: backport to v6.1.x coordinates Docs modeling no-changelog-entry-needed testing
Projects
None yet
Milestone
v6.1.1
2 participants
@pllim @neutrinoceros