Skip to content

Restore LMLSQFitter __call__ signature - #17164

Merged
astrofrog merged 1 commit into
astropy:mainfrom
larrybradley:lmlsqfitter-call
Oct 12, 2024
Merged

Restore LMLSQFitter __call__ signature#17164
astrofrog merged 1 commit into
astropy:mainfrom
larrybradley:lmlsqfitter-call

Conversation

@larrybradley

@larrybradley larrybradley commented Oct 11, 2024

Copy link
Copy Markdown
Member

Description

This PR restores the LMLSQFitter.__call__ signature that was changed in #16994.

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

@larrybradley larrybradley added modeling Affects-dev PRs and issues that do not impact an existing Astropy release no-changelog-entry-needed labels Oct 11, 2024
@larrybradley larrybradley added this to the v7.0.0 milestone Oct 11, 2024
@larrybradley
larrybradley requested a review from a team as a code owner October 11, 2024 20:19
@github-actions

github-actions Bot commented Oct 11, 2024

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.

pllim
pllim approved these changes Oct 11, 2024
View reviewed changes

@pllim pllim 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.

If it works for you, then LGTM. I'll let you decide if my approval is enough or you need others to review as well. Thanks!

@astrofrog
astrofrog merged commit a2d4b07 into astropy:main Oct 12, 2024
@nden

nden commented Oct 13, 2024

Copy link
Copy Markdown
Contributor

Am I right to think that the __call__ signature here was restored and that removed the Deprecation warning of the original PR but kept the change log for it? In other words the changelog for the deprecation is in the code but the deprecation warning is missing?

astrofrog
astrofrog reviewed Oct 13, 2024
View reviewed changes
Comment thread astropy/modeling/fitting.py
@@ -1674,7 +1687,18 @@ def __call__(self, model, *args, **kwargs):
AstropyDeprecationWarning,

@astrofrog astrofrog Oct 13, 2024

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.

@nden I think the deprecation warning is still here

@nden nden Oct 13, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hm, I don't see it on main. Or am confused and somehow looking at the wrong file.

@astrofrog astrofrog Oct 13, 2024

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.

See here:

astropy/astropy/modeling/fitting.py

Line 1724 in 11182c3

if model.has_bounds:

@mhvk

mhvk commented Oct 13, 2024

Copy link
Copy Markdown
Contributor

After this PR, most tests are failing on main. E.g., from https://github.com/astropy/astropy/actions/runs/11314917076/job/31465460014:

FAILED ../../.tox/py311-test-oldestdeps-alldeps-cov-clocale/lib/python3.11/site-packages/astropy/modeling/tests/test_fitters.py::TestWeightedFittingWithOutlierRemoval::test_2d_with_weights_with_sigma_clip[LMLSQFitter] - Failed: DID NOT WARN. No warnings of type (<class 'astropy.utils.exceptions.AstropyUserWarning'>,) were emitted.

I get the same problem locally.

No idea why the tests on the PR here seemed absolutely fine!

@dhomeier dhomeier mentioned this pull request Oct 13, 2024
Closed
1 task
@mhvk mhvk mentioned this pull request Oct 13, 2024
Closed
dhomeier
dhomeier reviewed Oct 13, 2024
View reviewed changes
Comment thread astropy/modeling/fitting.py
Comment on lines +1660 to +1673
@fitter_unit_support
def __call__(
self,
model,
x,
y,
z=None,
weights=None,
maxiter=DEFAULT_MAXITER,
acc=DEFAULT_ACC,
epsilon=DEFAULT_EPS,
estimate_jacobian=False,
filter_non_finite=False,
):

@dhomeier dhomeier Oct 13, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I suspect this had not been rebased to #17033 and thus the explicit signature is now missing the inplace kwarg that was added there.

@dhomeier dhomeier mentioned this pull request Oct 13, 2024
Merged
1 task
@larrybradley
larrybradley deleted the lmlsqfitter-call branch October 14, 2024 13:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@astrofrog astrofrog astrofrog approved these changes
@dhomeier dhomeier dhomeier left review comments
@nden nden nden left review comments
@pllim pllim pllim approved these changes

Assignees

No one assigned

Labels

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

Projects

None yet

Milestone

v7.0.0

Development

Successfully merging this pull request may close these issues.

6 participants

@larrybradley @nden @mhvk @astrofrog @dhomeier @pllim