Skip to content

TST: Disable Gaussian1DKernel in pickle test due to flakiness - #18596

Merged
larrybradley merged 1 commit into
astropy:mainfrom
pllim:disable-gauss1d-kern-test
Sep 5, 2025
Merged

TST: Disable Gaussian1DKernel in pickle test due to flakiness#18596
larrybradley merged 1 commit into
astropy:mainfrom
pllim:disable-gauss1d-kern-test

Conversation

@pllim

@pllim pllim commented Sep 4, 2025

Copy link
Copy Markdown
Member

Description

This pull request is to close #18569

  • 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 v7.1.1 milestone Sep 4, 2025
@pllim pllim added testing Bug Affects-dev PRs and issues that do not impact an existing Astropy release backport-v7.1.x labels Sep 4, 2025
@github-actions

github-actions Bot commented Sep 4, 2025

Copy link
Copy Markdown
Contributor

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 Bot commented Sep 4, 2025

Copy link
Copy Markdown
Contributor

👋 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
pllim marked this pull request as ready for review September 4, 2025 17:34
@pllim
pllim requested a review from larrybradley as a code owner September 4, 2025 17:34
@pllim
pllim requested a review from eerovaher September 4, 2025 17:35
larrybradley
larrybradley reviewed Sep 4, 2025
View reviewed changes
Comment thread astropy/convolution/tests/test_pickle.py Outdated
[
conv.CustomKernel(array=np.random.rand(15)),
conv.Gaussian1DKernel(1.0, x_size=5),
# conv.Gaussian1DKernel(1.0, x_size=5), # FIXME: Flaky failure

@larrybradley larrybradley Sep 4, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I thought @eerovaher reported that the flakiness originates in modeling. So shouldn't the "FIXME" be somewhere in modeling? Is the Gaussian1DKernel test the only one that is intermittently failing?

@pllim pllim Sep 4, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I would like the FIXME to be close to the code to be uncommented in the future. I can open a follow-up issue with more info after this is merged if that helps.

From what I see, seems like only Gaussian1DKernel in the failed log. But then again, I do not go read ALL the fail logs all the time, so I cannot give you absolute guaranteee.

@eerovaher eerovaher Sep 4, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

astropy.convolution.Gaussian1DKernel sometimes fails check_pickling_recovery() because astropy.modeling.functional_models.Gaussian1D sometimes fails it. Fixing the latter will automatically fix the former. It would be better if the comment in the test mentioned that.

The fundamental problem seems to be that modeling pickling tests don't use the general astropy.tests.helper.check_pickling_recovery() utility. The big difference between modeling tests and check_pickling_recovery() is that the former compares original and unpickled data with numpy.testing.assert_allclose(), whereas the latter requires strict equality. It is not clear to me why modeling chose not to use the general utility.

@pllim pllim Sep 4, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

OK I expanded the comment as you suggested. Looks good now?

@pllim
pllim force-pushed the disable-gauss1d-kern-test branch from 6dbb3f6 to 96be159 Compare September 4, 2025 21:06
larrybradley
larrybradley approved these changes Sep 5, 2025
View reviewed changes

@larrybradley larrybradley left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks, @pllim. Hopefully someone will fix the modeling tests.

@larrybradley
larrybradley merged commit 946496b into astropy:main Sep 5, 2025
meeseeksmachine pushed a commit to meeseeksmachine/astropy that referenced this pull request Sep 5, 2025
@larrybradley @meeseeksmachine
Backport PR astropy#18596: TST: Disable Gaussian1DKernel in pickle te…
ecd57be 
…st due to flakiness
@meeseeksmachine meeseeksmachine mentioned this pull request Sep 5, 2025
Merged
@pllim
pllim deleted the disable-gauss1d-kern-test branch September 5, 2025 17:46
@pllim pllim mentioned this pull request Sep 5, 2025
Open
1 task
@pllim

pllim commented Sep 5, 2025

Copy link
Copy Markdown
Member Author

I opened #18598 as follow-up issue. Feel free to add details as you desire. Thanks, all!

pllim added a commit that referenced this pull request Sep 5, 2025
@pllim
Merge pull request #18597 from meeseeksmachine/auto-backport-of-pr-18…
de3f6f9 
…596-on-v7.1.x

Backport PR #18596 on branch v7.1.x (TST: Disable Gaussian1DKernel in pickle test due to flakiness)
@Mohit-Ak Mohit-Ak mentioned this pull request Jul 13, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@larrybradley larrybradley larrybradley approved these changes
@eerovaher eerovaher Awaiting requested review from eerovaher

Assignees

No one assigned

Labels

Affects-dev PRs and issues that do not impact an existing Astropy release Bug convolution no-changelog-entry-needed testing

Projects

None yet

Milestone

v7.1.1

Development

Successfully merging this pull request may close these issues.

TST: Non-deterministic failures with AssertionError: Value of _parameters changed by pickling

3 participants

@pllim @larrybradley @eerovaher