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

Make _fitter_to_model_params and _model_to_fit_params public. #12585

Merged
merged 4 commits into from Dec 18, 2021
Merged

Make _fitter_to_model_params and _model_to_fit_params public. #12585

merged 4 commits into from Dec 18, 2021

Conversation

WilliamJamieson
Copy link
Contributor

Description

Use of the methods _fitter_to_model_params and _model_to_fit_params from astropy.modeling.fitting when creating custom fitters or subclassing fitters has come up a few times for me recently (and others see #11735). It is unfortunate that these functions are private because using them externally then posses some dangers. Since using these methods is sometimes necessary when creating one's own fitters, this PR makes these methods public to avoid the issues associated with using astropy private methods outside astropy.

Fixes #11735

Checklist for package maintainer(s)

This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.

  • 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 "When to rebase and squash commits".
  • 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.
  • 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 a milestone set? Milestone must be set but astropy-bot check might be missing; do not let the green checkmark fool you.
  • 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.

@WilliamJamieson
Copy link
Contributor Author

Tagging @nden, @perrygreenfield, and @mcara for review.

@pllim pllim added this to the v5.1 milestone Dec 10, 2021
@pllim pllim added the API change PRs and issues that change an existing API, possibly requiring a deprecation period label Dec 10, 2021
@WilliamJamieson WilliamJamieson force-pushed the feature/public_fitter_to_model_params branch from b866d17 to cf4b194 Compare December 14, 2021 19:55
nden
nden approved these changes Dec 17, 2021
View reviewed changes
Copy link
Contributor

@nden nden left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@nden nden merged commit befee5e into astropy:main Dec 18, 2021
@WilliamJamieson WilliamJamieson deleted the feature/public_fitter_to_model_params branch December 18, 2021 15:00
WilliamJamieson added a commit to WilliamJamieson/astropy that referenced this pull request Dec 20, 2021
@WilliamJamieson
Support for astropy#12585's api change.
8a26809
WilliamJamieson added a commit to WilliamJamieson/astropy that referenced this pull request Dec 20, 2021
@WilliamJamieson
Support for astropy#12585's api change.
8d12251
WilliamJamieson added a commit to WilliamJamieson/astropy that referenced this pull request Dec 21, 2021
@WilliamJamieson
Support for astropy#12585's api change.
c45af10
WilliamJamieson added a commit to WilliamJamieson/astropy that referenced this pull request Dec 21, 2021
@WilliamJamieson
Support for astropy#12585's api change.
cdceeb5
WilliamJamieson added a commit to WilliamJamieson/astropy that referenced this pull request Jan 3, 2022
@WilliamJamieson
Support for astropy#12585's api change.
bbb1948
WilliamJamieson added a commit to WilliamJamieson/astropy that referenced this pull request Jan 6, 2022
@WilliamJamieson
Support for astropy#12585's api change.
8f2530c
WilliamJamieson added a commit to WilliamJamieson/astropy that referenced this pull request Jan 10, 2022
@WilliamJamieson
Support for astropy#12585's api change.
d9cf4e0
WilliamJamieson added a commit to WilliamJamieson/astropy that referenced this pull request Jan 12, 2022
@WilliamJamieson
Support for astropy#12585's api change.
a48bf6a
WilliamJamieson added a commit to WilliamJamieson/astropy that referenced this pull request Jan 13, 2022
@WilliamJamieson
Support for astropy#12585's api change.
c7ae8ea
WilliamJamieson added a commit to WilliamJamieson/astropy that referenced this pull request Jan 27, 2022
@WilliamJamieson
Support for astropy#12585's api change.
65a21c0
WilliamJamieson added a commit to WilliamJamieson/astropy that referenced this pull request Feb 2, 2022
@WilliamJamieson
Support for astropy#12585's api change.
75b749a
WilliamJamieson added a commit to WilliamJamieson/astropy that referenced this pull request Feb 15, 2022
@WilliamJamieson
Support for astropy#12585's api change.
1b903de
WilliamJamieson added a commit to WilliamJamieson/astropy that referenced this pull request Feb 17, 2022
@WilliamJamieson
Support for astropy#12585's api change.
1d82ded
WilliamJamieson added a commit to WilliamJamieson/astropy that referenced this pull request Mar 2, 2022
@WilliamJamieson
Support for astropy#12585's api change.
d606a6e
WilliamJamieson added a commit to WilliamJamieson/astropy that referenced this pull request Mar 3, 2022
@WilliamJamieson
Support for astropy#12585's api change.
cbcd257
WilliamJamieson added a commit to WilliamJamieson/astropy that referenced this pull request Mar 8, 2022
@WilliamJamieson
Support for astropy#12585's api change.
b432051
WilliamJamieson added a commit to WilliamJamieson/astropy that referenced this pull request Mar 9, 2022
@WilliamJamieson
Support for astropy#12585's api change.
ce8578e
WilliamJamieson added a commit to WilliamJamieson/astropy that referenced this pull request Mar 9, 2022
@WilliamJamieson
Support for astropy#12585's api change.
77c5944
WilliamJamieson added a commit to WilliamJamieson/astropy that referenced this pull request Mar 10, 2022
@WilliamJamieson
Support for astropy#12585's api change.
1e3119d
WilliamJamieson added a commit to WilliamJamieson/astropy that referenced this pull request Mar 14, 2022
@WilliamJamieson
Support for astropy#12585's api change.
97b433c
WilliamJamieson added a commit to WilliamJamieson/astropy that referenced this pull request Mar 31, 2022
@WilliamJamieson
Support for astropy#12585's api change.
a49a3ca
WilliamJamieson added a commit to WilliamJamieson/astropy that referenced this pull request Apr 4, 2022
@WilliamJamieson
Support for astropy#12585's api change.
09d934a
WilliamJamieson added a commit to WilliamJamieson/astropy that referenced this pull request Apr 8, 2022
@WilliamJamieson
Support for astropy#12585's api change.
fca129b
WilliamJamieson added a commit to WilliamJamieson/astropy that referenced this pull request Apr 11, 2022
@WilliamJamieson
Support for astropy#12585's api change.
d3d062a
WilliamJamieson added a commit to WilliamJamieson/astropy that referenced this pull request Apr 19, 2022
@WilliamJamieson
Support for astropy#12585's api change.
7812ff5
WilliamJamieson added a commit to WilliamJamieson/astropy that referenced this pull request Apr 20, 2022
@WilliamJamieson
Added TFSLSQFitter
ed1a871 
Refactored fitter tests so that TRFLSQFitter identically to the LevMarLSQFitter.

Added bound calculation for TRFLSTFitter. One test needs to be corrected.

Provided a mechanism to fix issues with bounds and the fitter.

Moved fully duplicated code into common parent class

Moved objective_function to common parent class

Refactored param_cov computation

Refactored call method.

Added more TRFLSQFitter tests and fixed no-scipy testing bug

Expanded TRFLSQFitter tests

Added skip for fitting ArcSine and ArcCosine due to bad domains.

Added Changelog

Added TRFLSQFitter to __all__

Support for astropy#12585's api change.

Fix tolerance

isort changes

Fix for skipping tests correctly

Fix xfails

Fix codestyle

Fix broken CI due to scipy updates

Add documentation comments requested by nden.

Added more tests for TRFLSQFitter in test_fitters.py

Added more tests for TRFLSQFitter lm mode to remaining test suites

Added tests for TRFLSQFitter using dogbox method

Filter CI warnings from older scipy versions

Change to using separate fitter classes for each new fitter method
WilliamJamieson added a commit to WilliamJamieson/astropy that referenced this pull request Apr 20, 2022
@WilliamJamieson
Added TFSLSQFitter
348dbd1 
Refactored fitter tests so that TRFLSQFitter identically to the LevMarLSQFitter.

Added bound calculation for TRFLSTFitter. One test needs to be corrected.

Provided a mechanism to fix issues with bounds and the fitter.

Moved fully duplicated code into common parent class

Moved objective_function to common parent class

Refactored param_cov computation

Refactored call method.

Added more TRFLSQFitter tests and fixed no-scipy testing bug

Expanded TRFLSQFitter tests

Added skip for fitting ArcSine and ArcCosine due to bad domains.

Added Changelog

Added TRFLSQFitter to __all__

Support for astropy#12585's api change.

Fix tolerance

isort changes

Fix for skipping tests correctly

Fix xfails

Fix codestyle

Fix broken CI due to scipy updates

Add documentation comments requested by nden.

Added more tests for TRFLSQFitter in test_fitters.py

Added more tests for TRFLSQFitter lm mode to remaining test suites

Added tests for TRFLSQFitter using dogbox method

Filter CI warnings from older scipy versions

Change to using separate fitter classes for each new fitter method
WilliamJamieson added a commit to WilliamJamieson/astropy that referenced this pull request Apr 21, 2022
@WilliamJamieson
Added TFSLSQFitter
8a339ac 
Refactored fitter tests so that TRFLSQFitter identically to the LevMarLSQFitter.

Added bound calculation for TRFLSTFitter. One test needs to be corrected.

Provided a mechanism to fix issues with bounds and the fitter.

Moved fully duplicated code into common parent class

Moved objective_function to common parent class

Refactored param_cov computation

Refactored call method.

Added more TRFLSQFitter tests and fixed no-scipy testing bug

Expanded TRFLSQFitter tests

Added skip for fitting ArcSine and ArcCosine due to bad domains.

Added Changelog

Added TRFLSQFitter to __all__

Support for astropy#12585's api change.

Fix tolerance

isort changes

Fix for skipping tests correctly

Fix xfails

Fix codestyle

Fix broken CI due to scipy updates

Add documentation comments requested by nden.

Added more tests for TRFLSQFitter in test_fitters.py

Added more tests for TRFLSQFitter lm mode to remaining test suites

Added tests for TRFLSQFitter using dogbox method

Filter CI warnings from older scipy versions

Change to using separate fitter classes for each new fitter method
@ycopin ycopin mentioned this pull request Jul 4, 2022
Closed
dhomeier pushed a commit to dhomeier/astropy that referenced this pull request Oct 28, 2022
@WilliamJamieson @dhomeier
Added TFSLSQFitter
0eb9055 
Refactored fitter tests so that TRFLSQFitter identically to the LevMarLSQFitter.

Added bound calculation for TRFLSTFitter. One test needs to be corrected.

Provided a mechanism to fix issues with bounds and the fitter.

Moved fully duplicated code into common parent class

Moved objective_function to common parent class

Refactored param_cov computation

Refactored call method.

Added more TRFLSQFitter tests and fixed no-scipy testing bug

Expanded TRFLSQFitter tests

Added skip for fitting ArcSine and ArcCosine due to bad domains.

Added Changelog

Added TRFLSQFitter to __all__

Support for astropy#12585's api change.

Fix tolerance

isort changes

Fix for skipping tests correctly

Fix xfails

Fix codestyle

Fix broken CI due to scipy updates

Add documentation comments requested by nden.

Added more tests for TRFLSQFitter in test_fitters.py

Added more tests for TRFLSQFitter lm mode to remaining test suites

Added tests for TRFLSQFitter using dogbox method

Filter CI warnings from older scipy versions

Change to using separate fitter classes for each new fitter method
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@saimn saimn saimn left review comments

@pllim pllim pllim left review comments

@nden nden nden approved these changes

Assignees
No one assigned
Labels
API change PRs and issues that change an existing API, possibly requiring a deprecation period modeling
Projects
None yet
Milestone
v5.1
Development

Successfully merging this pull request may close these issues.

Make _fitter_to_model_params public available to all users
4 participants
@WilliamJamieson @saimn @nden @pllim