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.

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

100 refactor fitting for feature sets #101

Open
wants to merge 43 commits into
base: master
Choose a base branch
from

Conversation

ConnorPigg
Copy link
Collaborator

@ConnorPigg ConnorPigg commented Jun 20, 2019

Closes #100, Closes #103, and closes #99 by refactoring fitting to allow fitting to a tree that contains multiple properties.

Begins solution for #100 with an initial solution. Also adds
 various supporting features to ProfileProperty and PropertyDict.
Changes to the bayes testing in order to accomidate the change of
 the fitting to interpolate outside of the fitting process.
This includes a test for a new "feature_domain" property.
 This property is used to enable the testing of interpolated
 values prior to attempting to perform a fit.
This is done if errors are available. In either case weighting
 has a two norm of 1. Also updates modelling to not depend on
 properties having a length to use scalar properties.
Also allow PropertyDict.subset to take a single name since this is
 a potentially common occurance and if not handled correctly can
 provide strange results. Options were to throw an error or to
 correctly implement it.
This allows users to configure or adjust parameters prior to
performing the fits. Adjusts the tests to match.
Also removed "unusable" guess function which would not work with
the composite modelling used during fitting.
@ConnorPigg ConnorPigg requested a review from jmborr June 20, 2019 18:12
@ConnorPigg ConnorPigg marked this pull request as ready for review June 20, 2019 19:14
@jmborr
Copy link
Owner

jmborr commented Jun 20, 2019

@ConnorPigg please let me know when you think this PR is ready to be reviewed and I'll take a look

Copy link
Owner

@jmborr jmborr left a comment

Choose a reason for hiding this comment

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

review of properties.py

idpflex/properties.py Outdated Show resolved Hide resolved
idpflex/properties.py Outdated Show resolved Hide resolved
idpflex/properties.py Outdated Show resolved Hide resolved
idpflex/properties.py Outdated Show resolved Hide resolved
idpflex/properties.py Outdated Show resolved Hide resolved
idpflex/properties.py Show resolved Hide resolved
idpflex/properties.py Show resolved Hide resolved
Copy link
Owner

@jmborr jmborr left a comment

Choose a reason for hiding this comment

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

partial review of bayes.py

idpflex/bayes.py Outdated Show resolved Hide resolved
Also addresses additional documenation changes for PR.
Also closes #102 by allowing global minimization method choice.
This closes #102 by providing tests for using it.
Previously rewrote tests to exclude interpolation so undoing that
change. Also, include testing the centering aspect of the model.
However no testing has been completed and the "legacy"
multiproperty fitting has not been removed. Initial use with
`fitting_both.py` script indicates successful fitting.
Replaces references to the MultiPropertyModel class for the new
model creation. Adjustments to the test to reflect the change in
model creation interface. Tests for acceptance of general models
and tests for parameter naming are still to be completed.
@ConnorPigg ConnorPigg requested a review from jmborr June 26, 2019 17:46
This goes beyond the integration tests already present to test
specific expected behavior for the added general modelling.
This is to reflect that functions should not be directly passed
through to this parameter. It is expected that functions are
first converted to lmfit.Model classes or instances.
When Model instances are passed instead of classes, the object
was not properly handled which lead to each "use" of that model
template to overwrite the previous uses.
Attempt to be more "pythonic" by using try accept. Avoids using
`isinstance` to check types. Generalizes accepted types to single
object or iterables not just lists.
Copy link
Owner

@jmborr jmborr left a comment

Choose a reason for hiding this comment

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

partial review

idpflex/properties.py Outdated Show resolved Hide resolved
idpflex/properties.py Outdated Show resolved Hide resolved
idpflex/properties.py Outdated Show resolved Hide resolved
idpflex/properties.py Outdated Show resolved Hide resolved
idpflex/properties.py Outdated Show resolved Hide resolved
PropertyDict no longer expects `None` for names to resturn self.
Instead, expect user to pass self.keys() for a complete subset.
Copy link
Owner

@jmborr jmborr left a comment

Choose a reason for hiding this comment

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

partial review of bayes module

idpflex/properties.py Outdated Show resolved Hide resolved
idpflex/properties.py Outdated Show resolved Hide resolved
idpflex/properties.py Show resolved Hide resolved
idpflex/bayes.py Outdated Show resolved Hide resolved
idpflex/bayes.py Show resolved Hide resolved
idpflex/bayes.py Outdated Show resolved Hide resolved
idpflex/bayes.py Show resolved Hide resolved
idpflex/bayes.py Outdated Show resolved Hide resolved
idpflex/bayes.py Outdated Show resolved Hide resolved
idpflex/bayes.py Outdated Show resolved Hide resolved
Adds optional filtering when taking a subset of PropertyDict.
Adds periods to documentation. Change calls of `super` to use no
arguments. Attempt to optimize property interpolation by only creating
the interpolator once instead of on each call.
This leads to reliability that failed tests are not due to just
randomness. This raises the issue that the occasional failures
from the randomness should be classified, fixed, and have
dedicated tests. Hypothesis tests may be useful for this.
Allow user settings for interpolators to persist through property
data modification by changing how interpolators are created. These
settings are now saved into an attribute of the property.
This is to enforce structure probabilities sum to 1.
Also rewrote global minimization testing to actually make sense.
Restructure modelling to focus on linear "mx + b" models. Change
to using proportions that are converted to probabilies for
interpretation. Remove tabulated only modeling and instead allow
choice of tabulated modelling in the restructured modelling
options.
Updates were to improve clarity.
This involves being more verbose and adding tests for these
parameter hints. To simplify this, this changes the tabulated model
to use the same parameter names to simplify the logic to hard coded
parameter names.
The `use_tabulated` was not being passed down from create to depth
to the building block functions.
The difference places restrictions on the composite to not allow
simultaneous centering and interpolation to different qvalues.
The restriction is in place because from the experimental feature
vector it is difficult to determine what section should be used
as the new qvalues. Therefore require user to interpolate before
and simply allow centering in composite.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants