Skip to content

Implement custom_model_2d#1763

Merged
embray merged 9 commits into
astropy:masterfrom
embray:modeling/custom-model
Sep 3, 2014
Merged

Implement custom_model_2d#1763
embray merged 9 commits into
astropy:masterfrom
embray:modeling/custom-model

Conversation

@embray

@embray embray commented Jul 23, 2014

Copy link
Copy Markdown
Member

Since we have custom_model_1d, is there any reason we can't have custom_model_2d?

@embray

embray commented Nov 9, 2013

Copy link
Copy Markdown
Member

No, it's trivial. I'm actually thinking of replacing custom_model_1d with just custom_model. The only substantive difference is in the number of arguments the function takes. Right now we make the distinction because we name the arguments 'x', 'y', etc. which only matters so far as the documentation of the function signature. But we could still use 'x', 'y', 'z' for 1 <= d <= 3 and then just *args for higher-dimensional models should that even be useful to someone.

@embray

embray commented Nov 9, 2013

Copy link
Copy Markdown
Member

Same goes, by the way, for the Parametric1DModel and Parametric2DModel classes. I think when @adonath originally worked on these there was a bit more difference between them, but I made the base ParametricModel class more powerful, and as a result there's little to distinguish them except, again, the names and number of arguments to the __call__ methods and I guess the default n_input attribute.

@cdeil

cdeil commented Apr 4, 2014

Copy link
Copy Markdown
Member

@embray Issue #2274 would probably best be fixed by the generalisations you describe above.

And here's another bug in the current implementation of custom_model_1d (can't handle models with zero parameters) that would probably best be fixed by a rewrite with a generalised custom_model as you describe above:

>>> from astropy.modeling.models import custom_model_1d
>>> def my_model(x):
...     return 42 * x
>>> custom_model_1d(my_model)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/deil/code/astropy2/astropy/modeling/functional_models.py", line 1307, in custom_model_1d
    nparams = len(param_values)
TypeError: object of type 'NoneType' has no len()

@embray

embray commented Apr 4, 2014

Copy link
Copy Markdown
Member

Okay, I'll take a look at it. The fix for parameter-less models should be straightforward. I have a slew of other modeling changes in the pipeline that are working toward bashing down unnecessary complexities and distilling the functionality of the core model classes. See #2276 for one of the first in this series, but there are more on the way. It should make issues like this a little easier to deal with.

@embray embray self-assigned this Apr 18, 2014
@astrofrog

Copy link
Copy Markdown
Member Author

Since this is a feature request, I'm re-scheduling to 1.0.0

@astrofrog astrofrog modified the milestones: v1.0.0, v0.4.0 May 8, 2014
@astrofrog astrofrog mentioned this pull request Jul 17, 2014
14 tasks
@embray

embray commented Jul 23, 2014

Copy link
Copy Markdown
Member

Attached to this a PR to implement a custom_model function as I've promised in the past. So rather than custom_model_2d (or custom_model_1d for that matter) just use custom_model--the number of inputs and parameters is determined by the function syntax.

See the docstring for custom_model for an example of a 2D model.

This may seem frivolous to some, but it's a preview of other related changes to come.

This PR depends on #2580 which should be merged first.

@embray

embray commented Jul 24, 2014

Copy link
Copy Markdown
Member

Rebased, hopefully, with doc fixes.

@embray

embray commented Aug 7, 2014

Copy link
Copy Markdown
Member

Not sure why the doc build is failing but I don't think it's this PR's fault?

@embray embray mentioned this pull request Aug 7, 2014
embray added a commit to embray/astropy that referenced this pull request Aug 15, 2014
@embray

embray commented Aug 15, 2014

Copy link
Copy Markdown
Member

You can read an updated example of using custom_model here: http://embray.github.io/astropy/modeling/new.html#basic-custom-models (this was originally an example for custom_model_1d, just slightly updated with the new semantics).

The API docs for custom_model are rendered here: http://embray.github.io/astropy/api/astropy.modeling.custom_model.html#astropy.modeling.custom_model

@astrofrog

Copy link
Copy Markdown
Member Author

@embray - very nice! A few notes from trying this out - not all have to be acted on, but just reporting my experience:

  • I was trying to fit a 2-d data and so should have passed 3 arguments to the fit call after the model, but I did:
m = fit(m_init, x, y)

and the error message was:

TypeError: __call__() missing 1 required positional argument: 'y'

which is fine, but maybe it would be nice to get a more explicit error message? (the correct call is shown in the example below)

  • The example I ran is:
data = fits.getdata('msx.fits')
data = data[60:80, 70:90]

x = np.arange(data.shape[1])
y = np.arange(data.shape[0])
x, y = np.meshgrid(x, y)

# Define model
@custom_model
def gaussian_2d(x, y, amplitude=1., mu_x=10, mu_y=10, sigma=1):
    return amplitude * np.exp(-0.5 * ((x - mu_x) / sigma)**2) * np.exp(-0.5 * ((y - mu_y) / sigma)**2)

m_init = gaussian_2d()
fit = LevMarLSQFitter()
m = fit(m_init, x, y, data)

when I did this, I got the warning:

WARNING: The fit may be unsuccessful; check fit_info['message'] for more information. [astropy.modeling.fitting]

but as a user, I don't know what fit_info is. Could this be made more explicit? (It turns out the fit does actually look good).

Other than that, this works great, so 👍 from me! The issues above may be unrelated to this pull request, and just more general comments.

Comment thread astropy/modeling/core.py

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 like the way that you use custom_model as a function in the first example and as a decorator here. Out would be even better if there was a sentence in the text that both uses are possible.

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.

Yes, that's worth clarifying. I think I might also go ahead and make it so that the decorator can take arguments too (for example @custom_model(fit_deriv=...)

@embray

embray commented Aug 18, 2014

Copy link
Copy Markdown
Member

@astrofrog Those are issues in the fitter--totally unrelated to the custom_model implementation. I'm reworking fitters too but it's lower priority right now.

@embray

embray commented Aug 18, 2014

Copy link
Copy Markdown
Member

@astrofrog Or are you saying something in the sample documentation needs to be fixed too?

@astrofrog

Copy link
Copy Markdown
Member Author

@embray - if it's unrelated to this PR then ignore for now. Should I open issues for those or will they become moot once you rework the fitters anyway?

@embray embray force-pushed the modeling/custom-model branch from 534bb0c to 92f61ba Compare August 19, 2014 19:34
embray added a commit to embray/astropy that referenced this pull request Aug 19, 2014
… to take arguments when used as a decorator as well. I also clarified in the documentation that it can be used either as a decorator or a normal factory function.
@embray

embray commented Aug 19, 2014

Copy link
Copy Markdown
Member

You can open an issue if you want.

Comment thread astropy/modeling/core.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.

(Minor) This should be six.callable

@embray

embray commented Sep 3, 2014

Copy link
Copy Markdown
Member

Not sure why I didn't merge this already, but now it needs to be rebased..

….core, in the process updating it to support creating custom N-dimensional models. custom_model_1d is still kept for backward compat but will be deprecated.
…or subclasses, because then self.__class__ is not longer the correct base class in the super chain (in fact it leads to an infinite recursion). Turns it it suffices to attach __init__ and __call__ to the class after it has been created. Adds a test to ensure that this works.
…onger needs to include its own copy of the make_func_with_sig utility. I also renamed make_func_with_sig to make_function_with_signature as suggested here: astropy#2835 (comment)  Previously I thought the unabbreviated name was a little on the long side.  But on second thought I prefer the clarity here.
… to take arguments when used as a decorator as well. I also clarified in the documentation that it can be used either as a decorator or a normal factory function.
@embray embray force-pushed the modeling/custom-model branch from 92c3590 to 37d7c51 Compare September 3, 2014 22:19
@embray

embray commented Sep 3, 2014

Copy link
Copy Markdown
Member

Rebased. Waiting on Travis...

@embray

embray commented Sep 3, 2014

Copy link
Copy Markdown
Member

Docs build failed due to #2914 , but I confirmed locally that the docs built without warning (and prior the rebase the build passed) so I'm going ahead and merging.

embray added a commit that referenced this pull request Sep 3, 2014
@embray embray merged commit d0350f0 into astropy:master Sep 3, 2014
@embray embray deleted the modeling/custom-model branch September 3, 2014 23:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants