Move _model_to_fit_params out of ParametricModel#2264
Merged
Conversation
…n Fitter; this is the first step in separating concerns so that only Fitters are responsible for getting their inputs into the correct format they need. This allowed elimitating a lot of code rot in the process. This did require outright removing a handful of test cases, but those tests would be better handled within the fitter tests anyways--simply marking some parameters as constrained should have no bearing on a *model's* behavior by itself.
Member
Author
|
Test failures seem to be unrelated. Is matplotlib no longer supported with Python 3.2? |
Merged
Member
|
Looks good to me. |
Contributor
There was a problem hiding this comment.
When you get to separating optimizers from statistics, it becomes necessary for fitter_to_model_params to be a function. You may consider making model_to_fit_params a function as well.
(The reason is that LinearLSQFitter and NonLinearLSQFitter (called LevMarLSQFitter in #1914) can't be split into an optimizer and a statistic and so don't inherit from Fitter.)
Member
Author
There was a problem hiding this comment.
Right, I agree--this is going to be reworked anyway. Moving around the existing code is just a first step.
Contributor
|
@embray Other than that one comment this is good to merge. |
embray
added a commit
that referenced
this pull request
Apr 7, 2014
Move _model_to_fit_params out of ParametricModel
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Moved
ParametricModel._model_to_fit_paramsto being a static method inFitter; this is the first step in separating concerns so that only Fitters are responsible for getting their inputs into the correct format they need. This later might even move into a separate subclass ofFittersince this is particular for getting parameters into the form they need to be in for use withscipy.optimizefunctions.This allowed elimitating a lot of code rot in the process.
This did require outright removing a handful of test cases, but those tests would be better handled within the fitter tests anyways--simply marking some parameters as constrained should have no bearing on a model's behavior by itself. In a later PR I will add new tests that replace these ones--that ensure that parameter constraints are handled properly by all included fitters.
This will be the first of several PRs related to further decoupling the implementation of models from how they are used by fitters. Most of these PRs will have some dependency on each other but I will label when that is the case. I want to still split the work up into multiple PRs so it's easier to keep track of individual changes. Several of these changes can also be decoupled from each other if need be.