Skip to content

New models, model testing and Parametric2DModel base class - #1251

Merged
astrofrog merged 26 commits into
astropy:masterfrom
adonath:new_models_and_2D_base_class
Jul 29, 2013
Merged

astrofrog merged 26 commits into
astropy:masterfrom
adonath:new_models_and_2D_base_class

Conversation

@adonath

@adonath adonath commented Jul 12, 2013

Copy link
Copy Markdown
Member

This is a follow up PR to #1203.
At first I've implemented a new Parametric2DModel class, which is the base class for all 2D Models.

Than I've coded a few more 1D and 2D models. Including Box, Trapezoid, MexicanHat, mainly models
which are needed for the convolution kernel class (I will open a second PR for the convolution kernel class today...)

To have proper testing for all of these models, I've restructured "test_model.py" a bit. The main difference is, that
there is an additional file "model_lists.py" in the test folder, where all the models test values and test parameters are coded in a dictionary (this is more readable than simple lists in between the code and I assume it will get much longer in the future). Every model in this list is automatically tested for returning the right values, when called and for working with the NonLinearLSQFitter.

To make implementing new models still easier, I've written an IPython notebook, where you can calculate model
test values and derivatives with SymPy (here is a link). It also outputs a Latex string for the model docstring and shows
the model formula with symbols, which is a great test, to see if the model formula is correct. I think it would
be useful for everyone, who wants to implement new models. What would be the best place to put this notebook?
Currently it is just in the docs/modeling folder.

The work is "almost finished": Some models are only tested with one single value and most Latex model formulae
in the docstrings are missing. But this can be easily done with the Notebook.

I'm looking forward to any comments!

@nden

nden commented Jul 13, 2013

Copy link
Copy Markdown
Contributor

@adonath I'll review this early next week. I suggest this is merged before #1243 so I can resolve the conflicts there.

@nden

nden commented Jul 16, 2013

Copy link
Copy Markdown
Contributor

@adonath Could you add **constraints to the __init__ method of all models which you think may be fit with constraints. This will help with merging #1243.

@adonath

adonath commented Jul 22, 2013

Copy link
Copy Markdown
Member Author

@nden A short question: Is it currently possible to specify analytical derivatives for 2D models? How does the fitter handle this? I've made a short try and it didn't work...

@nden

nden commented Jul 22, 2013

Copy link
Copy Markdown
Contributor

@adonath I thought so. Although it was also possible to fit polynomials with the NonLinearLSQFitter before and that's not the case any more. I thought there's a test for that but maybe not. I'll have to look into this.
What error did you get?

@nden

nden commented Jul 22, 2013

Copy link
Copy Markdown
Contributor

I think the switch to column based derivative caused the regression with polynomials. We want to be able to fit linear models with non-linear fitters.

@nden

nden commented Jul 22, 2013

Copy link
Copy Markdown
Contributor

OK, once I fix the derivative of Poly2DModel I can use the NonLinearLSQFitter with it. I don't have a handy non-linear model with a derivative at the moment to test it. Can you post an example?

@adonath

adonath commented Jul 23, 2013

Copy link
Copy Markdown
Member Author

@nden Concerning the 2D derivatives:
Now it works. I've overlooked that the data was raveled for the fitting and you have to do the same of course with the derivative.

Concerning the Polynomials:
I've changed two things in the model derivatives and function calls:
(1) I changed the order of the function arguments and removed the dummy variables
(2) I changed the derivative from rows two columns
Do you work on this? I think I could do it also...

What kind of example do you need? E.g. there is the Gaussian1DModel which is an nonlinear Model with derivatives. A 2D model with dervivatives doesn't exist yet. But I have one example in another branch. Should I add it to this pull request? Thanks for the comments!

@nden

nden commented Jul 23, 2013

Copy link
Copy Markdown
Contributor

@adonath I'm working on something else, so please add the fix for polynomials to this PR. (1) and (2) is what I did to get it working yesterday as well but I didn't do it consistently.

I was asking if you have an example of a 2D non-linear model with derivatives which I can use for debugging but since it works, that's fine, no need to post one now.

How do you want to proceed with #1243 ? I was thinking we can merge this one first and then #1243 but I'm not sure how much other work you are doing. Would it be better to wait even longer?

@adonath

adonath commented Jul 23, 2013

Copy link
Copy Markdown
Member Author

I think this PR is almost finished. I will fix the polynomials and extend the documentation a bit (add some Latex formulae and "See Also" sections in the docstrings etc.). I will also add a new BetaModel. But this will be done by the end of the week.

@adonath

adonath commented Jul 26, 2013

Copy link
Copy Markdown
Member Author

I think this PR is finished. @nden Would you take a look at the changes?

Comment thread astropy/modeling/tests/test_models.py Outdated

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.

Minor: these should be relative imports

@astrofrog

Copy link
Copy Markdown
Member

Apart from a couple of minor comments, this looks good to me.

@adonath adonath closed this Jul 29, 2013
@astrofrog

Copy link
Copy Markdown
Member

@adonath - why did you close this pull request?

EDIT: I'm re-opening this since I'll assume this was accidental

@astrofrog astrofrog reopened this Jul 29, 2013
@adonath

adonath commented Jul 29, 2013

Copy link
Copy Markdown
Member Author

Thanks for reopening! I don't know why it was closed...

@astrofrog

Copy link
Copy Markdown
Member

No problem!

@nden - I think this is ready for a final review!

Comment thread astropy/modeling/fitting.py Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@adonath I was hoping to not have any models related imports in the fitting module and keep models and fitting completely separate. One reason is that in the future this may introduce more model specific dependencies. So would it make more sense to make col_deriv an attribute of ParametricModel with a default value of 1 and set to 0 in PolynomialModel and OrthogPolyBase? Then a fitter would use model.col_deriv.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This makes totally sense to me. I will change this, as you proposed...

@nden

nden commented Jul 29, 2013

Copy link
Copy Markdown
Contributor

I think this is great. After the col_deriv issue is cleared I think this can be merged. Thanks for the work @adonath .

@astrofrog

Copy link
Copy Markdown
Member

@nden - can you confirm whether this looks ok to you now?

@nden

nden commented Jul 29, 2013

Copy link
Copy Markdown
Contributor

@astrofrog Looks good to merge.

astrofrog added a commit that referenced this pull request Jul 29, 2013
New models, model testing and Parametric2DModel base class
@astrofrog
astrofrog merged commit 3293ea5 into astropy:master Jul 29, 2013
@astrofrog

Copy link
Copy Markdown
Member

Thanks @adonath!

@astrofrog

Copy link
Copy Markdown
Member

And thanks @nden for reviewing this :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants