Added a new power-law ParametricModel#1048
Conversation
There was a problem hiding this comment.
Just a picky thing, but the general recommendation here is to use super(). It should be fine here to do super(PowerLawModel, self).__init__(self.param_names, ndim=1, outdim=1, param_dim=param_dim)
There was a problem hiding this comment.
OK, that's a good idea - I think that should be added as a general recommendation in the models docs (which I have open as another PR, so I'll make that edit in both places)
There was a problem hiding this comment.
There is already a recommendation about that in the general developer guidelines. It's easy to miss though.
|
@keflavich Some Travis configurations don't have scipy installed and the test is failing in these with ImportError. Look at test_fitters.py for how to import scipy and mark tests which require it to be skipped when it is not available. |
There was a problem hiding this comment.
Set self.deriv = noderiv here or just name the method deriv instead of noderiv. Otherwise the derivatives will be estimated.
You probably looked at how the Gaussian1DModel handles derivatives but it is misleading. There I wanted to show all possible ways of handling derivatives and also use it for testing/debugging. So Gaussian1D can be fitted without derivatives, with a function to calculate the derivatives supplied by a user or using the class method to do this. We probably don't want to be that "flexible" so it should be changed some time in the future to use the method in the class.
With this fixed and when the tests pass I think it's good.
|
@nden - Oops! I had that in there from some tests I was running where I wanted to check the numerical derivatives. @iguananaut - thanks, fixed that |
|
The last test failure is because the fit is actually bad. |
|
@keflavich Could you please rebase this |
Old commits: * fix models reference in test and use super to init * needed to add powerlaw to __all__ * need to use parameters.Parameter * power law model only takes 2 pars * tests still fail, but the error no longer makes any sense * fixed the tests. Sigh. * skip test if no scipy * use the derivative! * corrected deriv function (was correct at some time earlier; must have forgotten to pull...)
|
Note that in keflavich#2 I added a docstring for the model class itself. |
Add a docstring to PowerLawModel
|
@keflavich I added a PR against your powerlaw-model branch but I am seeing a lot of changes in it probably due to the rebase. I'll make a PR against master to try to avoid this. |
Of course, not all models should be built-in, but I think a power-law is a simple and common enough one that it ought to be included.
I wrote this PR in part to get some practice working with
models; any feedback on style would be helpful.