Simplified implementing new models #980 #1203

adonath · 2013-06-24T17:46:06Z

Hi everyone,

during the last week I've worked on simplifying the implementation of new models.
So far this only works with 1D models, but I'd like to get some feedback, before I continue working on this.

At first I've introduced a new Parametric1DModel class, which is the base class for all
parametric 1D models. It mainly implements the appropriate call() function and does some "bookkeeping" in the constructor, to check whether analytical derivatives are implemented etc. The model parameters are currently set automatically by getting the
arguments from the model's constructor. I wanted a way to avoid setting all model parameters "by hand", as the Parameter class may still change.

Furthermore I've made a few changes in the models deriv() function. So far it used a dictionary and list comprehension for the derivatives, which I changed to an ordinary list. This which should be faster. Scipy's lsqr fitter has an option "col_deriv" which can be set, when the derivatives are given across the columns. So I removed the transpose step in deriv(). Finally I've changed the function call of deriv(), so that no dummy variables are necessary. It is only important that the order of parameters is consistent in the model definition. I hope that I've not overlooked something, which made this things necessary. But so far all tests passed.

I've implemented a few simple new models. Among a "Custom1DModel" which can be used to create new models "on the fly" just from a function. This may be useful when
working with astropy interactively.

Since this is my first "real" pull request, I would be happy for any kind of feedback! Thanks in advance!

astrofrog · 2013-06-24T22:32:52Z

@adonath - thanks for the PR! I will review this carefully tomorrow, but in the mean time, I notice there are two classes that are currently commented out - if they get replaced by other code then they should just be removed rather than commented (or if they are commented by mistake, they should be uncommented).

Once this is done, maybe @nden can review this too?

nden · 2013-06-25T02:30:59Z

Yes, I'll review it soon.

astrofrog · 2013-06-25T08:34:40Z

@adonath - thanks! one more request - can you move Gaussian1DModel back before Gaussian2DModel so that we can see the actual changes in Gaussian1DModel? (otherwise it's hard to see because it's all marked as deleted/added)

astrofrog · 2013-06-25T09:04:56Z

@adonath - thanks, and one more request - I think this will need to be rebased (you can see it says 'we can't automatically merge this pull request above' - do you need instructions for how to do this?

adonath · 2013-06-25T09:26:14Z

Hi Tom,

thanks for the hints! Yes, I could need intructions how to do a
rebase, I've never done that before.

Best Axel

Am Di 25 Jun 2013 10:34:43 CEST schrieb Thomas Robitaille:

@adonath https://github.com/adonath - thanks! one more request - can
you move |Gaussian1DModel| back before |Gaussian2DModel| so that we
can see the actual changes in |Gaussian1DModel|? (otherwise it's hard
to see because it's all marked as deleted/added)

—
Reply to this email directly or view it on GitHub
#1203 (comment).

astrofrog · 2013-06-25T09:33:19Z

@adonath - here are instructions:

http://docs.astropy.org/en/stable/development/workflow/development_workflow.html#rebase-on-trunk

Basically:

git fetch upstream
git rebase upstream/master
# resolve conflicts, then git add the conflicted files, then git rebase --continue, and you may have to do this several times
git push -f origin your-branch

However, before you start, make a backup of the folder on your computer, in case you mess up! If you have issues, we can do it over chat or hangout later - just let me know.

astrofrog · 2013-06-25T12:22:21Z

@adonath - I noticed you managed to rebase the branch - however, it looks like the conflicts were not dealt with cleanly (in part due to the incomplete documentation on rebasing). The idea is that if there is a commit that conflicts, then the conflicted files will contain something like

<<<<<<< HEAD
from .utils import InputParameterError
=======
from .utils import InputParameterError, ModelDefinitonError
from .utils import InputParameterError, ModelDefinitionError
from iminuit.util import param_name
>>>>>>> Changes in Paramteric1DModel class and minor changes related to deriv()

but before you git add the file, you need to ensure that there are no longer these kinds of chunks of code in the files - you need to 'resolve' the conflict, which means you should replace the above block with the new block of code, which may in some cases be a combination of the two parts outlined above. Once that is done, you can git add, then proceed. Does this make sense?

Do you still have a backup of the code as it was before the rebase so that we can try it again? (if you want we can do it interactively on chat or hangout).

Another alternative if rebasing becomes too much of a pain is to simply start from a new clean branch from the upstream master, and copy over the files are you want them in their final state in this PR, then just add them in a single commit and force push to the pull request. You will lose the commit history if you do this, but on the plus side the history will be cleaner and not have residual conflict markers in it. Does this make sense?

astrofrog · 2013-06-25T12:23:52Z

By the way, it's always hard to manage a rebase the first time, so no worries! It takes a few times to get used to, especially when there are conflicts :)

adonath · 2013-06-25T13:38:55Z

I've created a new branch from the current trunk and copied my changes,
as you proposed. I hope that resolved the issues.

Am Di 25 Jun 2013 14:23:56 CEST schrieb Thomas Robitaille:

By the way, it's always hard to manage a rebase the first time, so no
worries! It takes a few times to get used to, especially when there
are conflicts :)

—
Reply to this email directly or view it on GitHub
#1203 (comment).

astrofrog · 2013-06-25T13:40:49Z

@adonath - yes, this looks good and ready for review! Next time you need to do a rebase, let me know and we can do it together over a Hangout (it's good experience to have)

astrofrog · 2013-06-25T13:55:50Z

astropy/modeling/core.py

+
+    def __init__(self, param_dict, **cons):
+        # Get parameter dimension
+        param_dim = np.size(param_dict[self.param_names[0]])


I have to admit I don't quite understand the concept of some of the parameters having multiple (as opposed to scalar) values - for example amplitude in the Gaussian. What is the intent of this? (this is more of a question for @nden). If it is the case that parameters can have array values, what are the rules? Should all parameters have array values? If so, maybe we should sanity-check that here?

As far as I understood, the concept of parameters as arrays is, that the models can be evaluated with several parameter sets. So parameters can be scalar, lists or numpy arrays. The dimension of the parameter is stored in "param_dim". I have made a short check and np.size() should be save, it gives the appropriate value for scalars as well as lists and arrays.

I will review the PR later today but just to answer this question now (I've been asked this many times).
The intent of multiple parameters is to be able to simultaneously evaluate (or fit in some cases) the same type of model on multiple data sets. A good example is a cube with up-the-ramp IR data and performing a linearity correction. Fitting of polynomials with the LinearLSQFitter can be performed simultaneously and this is a huge improvement over looping through all pixels.

The current rules are:

linear and non-linear models can be evaluated simultaneously

linear models can be fit simultaneously if the the LinearLSQFitter is used

because of the nature of non-linear fitting algorithms non-linear models can be fit currently only one at a time
(But may be this can be changed in the future by using multiple processes?)

This is in contrast of having a parameter of type array. Parameters of type array are supported (although there's no model like this, only a test case in test_models.py.) If a parameter is an array and param_dim is > 1, the parameter is going to be a list of arrays.
However, the parameters attribute of a model is always a flat list. It's real purpose is to communicate with the fitter.

Does this make sense?

Ah, I see - so in cases where one parameter is an array, should all of them be arrays? If not, and it's possible for some to be scalars and some arrays, then you can't just assume the first is going to give you the dimension - you would need to loop through all the parameters to check that all those that are arrays/lists have the same dimension and save that as the dimension.

It is possible (in theory) to have mixed parameters (one array and one scalar). But say param_dim =2, then each parameter is a list, the first parameter is a list of 2 arrays and the second a list of 2 scalars. So checking the first one is sufficient.
If the user makes a mistake and gives 2 arrays but 1 scalar, I think this will be caught by the Parameter class but I have to double check.

astrofrog · 2013-06-25T13:59:51Z

This does look like it reduces the amount of duplicated code when writing new models, so I think this is ready for @nden to review.

We should also ensure that the Travis tests are passing.

nden · 2013-06-25T22:49:22Z

astropy/modeling/core.py

@@ -47,6 +47,7 @@
 from . import constraints
 from .utils import InputParameterError

+
 __all__ = ['Model', 'ParametricModel', 'PCompositeModel', 'SCompositeModel',
           'LabeledInput', '_convert_input', '_convert_output']



Can you add Parametric1DModel to __all__.

nden · 2013-06-26T03:17:26Z

@adonath The documentation build is failing because some of the examples use fwhm parameter for GaussianModel. Once this is fixed I think this is good. Thanks for the contribution.

adonath · 2013-06-26T14:46:16Z

@nden Thanks for the review and annotations. I will fix the documentation and the other points you have mentioned. So we keep the evaluataion of the model with "self.param_sets"?
I have still one further question:
The Gaussian1DModel has a "jacobian_func" argument. What is it exactly for? If the analytical derivatives are implemented, I don't see why it should be necessary. Or am I missing something? One could add a flag to the model base class, which indicates whether to use the analytical derivatives or the intrinsic estimation, which may be useful for other models too.

nden · 2013-06-26T15:14:31Z

@adonath Yeah we keep param_sets for now but it would be good to use the parameters list eventually.

The jacobian_func pararmeter has three possible values. If None - the analytical derivative is used. If 'estimated' - the fitter uses the intrinsic derivative estimation and if a callable is passed it is assumed the user provided their own deriv function. This last option may not be necessary in which case it is used as the flag you are proposing. Or did you mean something else? I am fine with removing the option of passing a callable.

adonath · 2013-06-26T15:39:05Z

@nden That was what I meant. Thanks for the quick answer!

astrofrog · 2013-06-30T12:30:32Z

@adonath - once you've fixed the Travis build, please leave a comment to let me know, and I can merge this since @nden said it was fine - thanks!

adonath · 2013-07-05T15:40:26Z

I've fixed the discussed issues and added new tests. The Travis builds passed. So is it ready to merge?

nden · 2013-07-05T16:05:34Z

astropy/modeling/fitting.py

-        if self._model.deriv is None:
-            self.dfunc = None
-        else:
-            self.dfunc = self._wrap_deriv


@adonath I am wondering what the reason for removing this section is. I see it was moved to the call function of NonLinearLSQFitter but it was meant to be applicable to all fitters. The SLSQPFitter can also be passed an analytical derivative and supposedly other fitters in the future should be able to take one too. Isn't moving this section of the code to the NonLinearLSQFitter only going to make this impossible?

You're right of course, so far it works only for NonLinearLSQFitter. I had two ideas in mind:
(1) This option was only available for Gaussian1DModel, but I thought it could be reasonable to have it for
all models, thats why I moved this option to the Fitter class.
(2) When you're fitting data, you may want to check if the result changes, whether you take the analytical
or estimated derivative (especially when you're working in an interactive shell). Having this option in the call function, you don't have to setup a new Fitter instance to change the "derivative mode". That was the only reason for putting it in the call function.

Is this reasonable? Thanks for checking the changes!

OK, that sounds good. Changing a parameter is better. Were you able to get this working with the other fitter?

I've had a look on the code again and it seems that currently only the NonLinearLSQFitter uses the analytical
derivative. All the other fitters don't, except for the LinearLSQFitter to set up the coefficient matrix. In principle the other Scipy functions take a derivative function, but currently this seems not to be implemented for the JointFitter and SLSQPFitter: No analytical derivative is passed to the minimizers and also the wrappers for the derivatives, that manage the constraints etc. are missing. Is this correct?
At first sight the handling of constraints seemed quite complicated, so I didn't want to work on this...

@adonath I remembered I had a problem passing a derivative to slsqp and wondered if you've tried it. But that's fine, I'll work on it. I am also working on a largish PR to simplify handling of constraints. Thanks for this work.

nden · 2013-07-05T18:31:34Z

astropy/modeling/functional_models.py

-        Either fwhm or stddev must be specified
-    fwhm : float
-        Full width at half maximum
-        Either fwhm or stddev must be specified
    jacobian_func : callable, 'estimated' or None
        if callable - a function to compute the Jacobian of
        func with derivatives across the rows.
        if 'estimated' - the Jacobian will be estimated
        if None - if the model has a deriv method, it will be used,
        if not the Jacobian will be estimated.


Remove jacobian_func from the Parameters section

nden · 2013-07-08T12:48:16Z

@astrofrog This looks good to me, ready to merge?

astrofrog · 2013-07-08T15:17:22Z

@nden - yes, will merge now

@adonath - thanks for the PR!

Simplified implementing new models #980

astrofrog · 2013-07-08T15:18:43Z

@adonath - for any other changes or adding tests, just open a new PR, but I wanted to make sure this was merged so that the main code doesn't conflict with any other pull requests in progress.

adonath mentioned this pull request Jun 24, 2013

Make creating new simple models more user-friendly #980

Closed

astrofrog reviewed Jun 25, 2013
View reviewed changes

wkerzendorf mentioned this pull request Jun 25, 2013

added new model parameter API. astropy/astropy-api#9

Open

nden reviewed Jun 25, 2013
View reviewed changes

adonath added 6 commits July 4, 2013 15:53

Simplified creating new models astropy#980

8f61867

WIP

59cd4a6

WIP

b00b6f9

Fixed the points discussed in astropy#1203

46cc8ac

Removed **kwargs in LSQFitter constructor

fb4b5c7

Fixed docstring error in Custom1DModel

38aaa50

nden reviewed Jul 5, 2013
View reviewed changes

Removed jacobian_func and added estimate_jacobian to docstring

a6f74dc

astrofrog added a commit that referenced this pull request Jul 8, 2013

Merge pull request #1203 from adonath/new_models_#980_and_#981

95a9e7f

Simplified implementing new models #980

astrofrog merged commit 95a9e7f into astropy:master Jul 8, 2013

adonath mentioned this pull request Jul 12, 2013

New models, model testing and Parametric2DModel base class #1251

Merged

jdnc mentioned this pull request Jul 12, 2013

Add TableList class to astroquery.utils astropy/astroquery#128

Merged

pllim mentioned this pull request Nov 3, 2016

Add Planar2d model #5456

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplified implementing new models #980 #1203

Simplified implementing new models #980 #1203

adonath commented Jun 24, 2013

astrofrog commented Jun 24, 2013

nden commented Jun 25, 2013

astrofrog commented Jun 25, 2013

astrofrog commented Jun 25, 2013

adonath commented Jun 25, 2013

astrofrog commented Jun 25, 2013

astrofrog commented Jun 25, 2013

astrofrog commented Jun 25, 2013

adonath commented Jun 25, 2013

astrofrog commented Jun 25, 2013

astrofrog Jun 25, 2013

adonath Jun 25, 2013

nden Jun 25, 2013

astrofrog Jun 25, 2013

nden Jun 25, 2013

astrofrog commented Jun 25, 2013

nden Jun 25, 2013

nden commented Jun 26, 2013

adonath commented Jun 26, 2013

nden commented Jun 26, 2013

adonath commented Jun 26, 2013

astrofrog commented Jun 30, 2013

adonath commented Jul 5, 2013

nden Jul 5, 2013

adonath Jul 5, 2013

nden Jul 5, 2013

adonath Jul 6, 2013

nden Jul 8, 2013

nden Jul 5, 2013

nden commented Jul 8, 2013

astrofrog commented Jul 8, 2013

astrofrog commented Jul 8, 2013

Simplified implementing new models #980 #1203

Simplified implementing new models #980 #1203

Conversation

adonath commented Jun 24, 2013

astrofrog commented Jun 24, 2013

nden commented Jun 25, 2013

astrofrog commented Jun 25, 2013

astrofrog commented Jun 25, 2013

adonath commented Jun 25, 2013

astrofrog commented Jun 25, 2013

astrofrog commented Jun 25, 2013

astrofrog commented Jun 25, 2013

adonath commented Jun 25, 2013

astrofrog commented Jun 25, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astrofrog commented Jun 25, 2013

Choose a reason for hiding this comment

nden commented Jun 26, 2013

adonath commented Jun 26, 2013

nden commented Jun 26, 2013

adonath commented Jun 26, 2013

astrofrog commented Jun 30, 2013

adonath commented Jul 5, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nden commented Jul 8, 2013

astrofrog commented Jul 8, 2013

astrofrog commented Jul 8, 2013