Skip to content

Refactor constraints - #1243

Merged
embray merged 8 commits into
astropy:masterfrom
nden:refactor-constraints
Aug 6, 2013
Merged

Refactor constraints#1243
embray merged 8 commits into
astropy:masterfrom
nden:refactor-constraints

Conversation

@nden

@nden nden commented Jul 10, 2013

Copy link
Copy Markdown
Contributor

This is a somewhat larger PR as it touches almost every module but the goal is to simplify and improve handling of constraints. The added tests hopefully will make it easier to further refactor the code or provide alternate implementations.

The biggest change is that the Constraints class was eliminated. As before constraints are properties of parameters. Fitters have a copy / list of different types of constraints (bounds, fixed, ...). These lists are created once but used in fitting iterations. Before starting a fit, fitters update their lists of constraints to pick up any changes made to the model. Models have a convenience getter property for each type of constraint which returns the current constraints on a model. For example:

g1=models.Gaussian1DModel(10, 2, 1, fixed={'amplitude': True})
g1.fixed
Out[6]: {'amplitude': True, 'mean': False, 'stddev': False}

gfit= fitting.NonLinearLSQFitter(g1)
gfit.fixed
Out[4]: [True, False, False]

gfit.bounds
Out[5]:
[(-1000000000000.0, 1000000000000.0),
 (-1000000000000.0, 1000000000000.0),
 (-1000000000000.0, 1000000000000.0)]

This required a change to the Parameter class which doesn't inherit from list any more. A new attribute Parameter.value was added and is mostly transparent to end users.
This change fixed the problem with losing the constraints after a fit. In the previous implementation, when a fitter updated the parameters, it did not update the previously set constraints and they were lost. Now a fitter updates the value attribute only and all other attributes are kept.

This PR addresses problems raised in issues #1064 , #1218. #1078

I also have a few questions on how to proceed

  • What value is it best to use to reset bounds? Internally the default values for bounds are (-1012, 1012) and they populate the min and max attributes of a parameter. Suppose a model was initialized with some bounds. What is the best way to reset them. For example, using gauss.mean.max = 10**12 to reset the value seems not very intuitive. Is gauss.mean.max = None acceptable?
  • What is a good way to include a standard docstring about constraints in all models?
    The constraints docstrings are currently included in ParametricModel and are not visible as individual models's docstrings. If I separate them as a standalone string, is there a way to attach them to a class docstring (all my attempts to append them to doc failed)?

embray
embray reviewed Jul 11, 2013
View reviewed changes
Comment thread astropy/modeling/core.py Outdated

@embray embray Jul 11, 2013

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 realize this is more consistent with other classes in this module, but I'm not entirely clear why **cons is used rather than naming the constraint arguments explicitly. Subclasses could use **cons for brevity, but I thought it made sense to have them explicit at least in the base class.

If nothing else, I would raise a TypeError if any unrecognized constraints are passed in. Currently if one mistyped a constraint argument it would just be ignored...

@nden nden Jul 11, 2013

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I used **constraints in the subclasses so that it's clear what goes in the dictionary. And I want to attach a common docstring to all models which support constraints explaining the details.

But you make a very good point about unrecognised constraints, I'll make a check.

@embray

embray commented Jul 11, 2013

Copy link
Copy Markdown
Member

This looks great--I had a feeling the Constraints class was superfluous but I wasn't sure enough.

As for a value to reset the bounds to a default I think something like g.mean.max = None makes sense.

As for the question of appending to a docstring the easiest way (though not terribly pretty) is to just do it after the class definition:

class Blah(object):
    """
    docs...
    """

Blah.__doc__ += addendum

But a nicer way might be to do it in a metaclass.

@nden

nden commented Jul 11, 2013

Copy link
Copy Markdown
Contributor Author

@iguananaut Thanks for looking at this and for the suggestion about docstrings. I'll finish the changes and push them.

@nden nden mentioned this pull request Jul 13, 2013
Merged
@nden

nden commented Jul 31, 2013

Copy link
Copy Markdown
Contributor Author

The one failing test here is in astropy.wcs and is not related.
Any other comments on this PR? Any objections to merging it as it is? I think the failing test should be changed to use numpy.random.RandomState and I can do this in a separate PR.

@nden

nden commented Jul 31, 2013

Copy link
Copy Markdown
Contributor Author

@lpsinger @mdboom What do you think about making test_world2pix use numpy.random.RandomState and merging this one as it is?

@astrofrog

astrofrog commented Jul 31, 2013

Copy link
Copy Markdown
Member

@nden - I agree that it should use RandomState. This one can be merged as-is once it has been reviewed (I restarted the job and it passed now).

@astrofrog

astrofrog commented Jul 31, 2013

Copy link
Copy Markdown
Member

Maybe @iguananaut could give this a final review since he has already looked at an earlier version?

@nden

nden commented Jul 31, 2013

Copy link
Copy Markdown
Contributor Author

@astrofrog thanks

embray
embray reviewed Aug 1, 2013
View reviewed changes
Comment thread astropy/modeling/fitting.py

@embray embray Aug 1, 2013

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.

There isn't really any reason to copy the array b here, but it's a small list so it doesn't really matter much.

@nden

nden commented Aug 5, 2013

Copy link
Copy Markdown
Contributor Author

Can anyone restart travis? I'd like to merge this soon.

@astrofrog

astrofrog commented Aug 5, 2013

Copy link
Copy Markdown
Member

@nden - I've just restarted it

@astrofrog

astrofrog commented Aug 5, 2013

Copy link
Copy Markdown
Member

I think that the failing test has now been fixed by @mdboom in master. Either way, once this is ready to merge, let's just ignore the Travis status.

@nden

nden commented Aug 5, 2013

Copy link
Copy Markdown
Contributor Author

@astrofrog Thanks! There's a separate PR #1312 which attempts to to fix the current failing test but it's failing as well. I think this is ready to merge unless @embray has other comments.

embray added a commit that referenced this pull request Aug 6, 2013
@embray
Merge pull request #1243 from nden/refactor-constraints
6e636cf 
Refactor constraints
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

modeling

Projects

None yet

Milestone

v0.3.0

Development

Successfully merging this pull request may close these issues.

4 participants

@nden @embray @astrofrog @pllim