Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Beta1D model #6374

Closed
wants to merge 8 commits into from
Closed

Add Beta1D model #6374

wants to merge 8 commits into from

Conversation

mirca
Copy link
Member

@mirca mirca commented Jul 19, 2017

cc @pllim @eteq
First time contributing in modeling.models, let me know if I'm missing something

@astropy-bot
Copy link

astropy-bot bot commented Jul 19, 2017

Hi there @mirca 👋 - thanks for the pull request! I'm just a friendly 🤖 that checks for issues related to the changelog and making sure that this pull request is milestoned and labelled correctly. This is mainly intended for the maintainers, so if you are not a maintainer you can ignore this, and a maintainer will let you know if any action is required on your part 😃.

Everything looks good from my point of view! 👍

Parameters
----------
amplitude : float
Amplitude at x=xpos
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a . at the end of the definition in all docstrings.

d_xpos = (-2 * amplitude * (-3 * beta + .5) * (1 + ((x - xpos) / r0)
** 2) ** (-3 * beta - .5) * (x - xpos) / r0 ** 2)
d_amplitude = (1 + ((x - xpos) / r0) ** 2) ** (-3 * beta + .5)

Copy link
Contributor

Choose a reason for hiding this comment

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

The same terms are computed many times. For those that makes sense could you compute them once at the beginning and reuse that.

@nden nden added the modeling label Jul 19, 2017
@nden nden added this to the v3.0.0 milestone Jul 19, 2017
@nden
Copy link
Contributor

nden commented Jul 19, 2017

@mirca Thanks for adding this model. Please add also a fitting test if possible,

Copy link
Member

@pllim pllim left a comment

Choose a reason for hiding this comment

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

Need a change log.

Also you don't need to write new functions for the testing. Most of the models are tested in https://github.com/astropy/astropy/blob/master/astropy/modeling/tests/example_models.py .

@pllim
Copy link
Member

pllim commented Jul 19, 2017

Also, I wonder if this should really belong in powerlaws.py or not... 🤔

@mirca
Copy link
Member Author

mirca commented Jul 20, 2017

import numpy as np
from astropy.modeling import models
from astropy.modeling.fitting import LevMarLSQFitter
from matplotlib import pyplot as plt

x = np.linspace(0, 5, 50)
noise = np.random.normal(loc=0, scale=0.05, size=len(x))
fake_data = models.Beta1D(xpos=1)(x) + noise

plt.plot(x, fake_data, 'x')

# initial guess model
f0_model = models.Beta1D(amplitude=1.5, beta=1.1, r0=0.75, xpos=0.9)
fitter = LevMarLSQFitter()
f_model = fitter(f0_model, x, fake_data)

plt.plot(x, f_model(x), '-')
print(f_model)
print(np.sqrt(np.diag(fitter.fit_info['param_cov'])))
plt.show()

Model: Beta1D
Inputs: ('x',)
Outputs: ('y',)
Model set size: 1
Parameters:
      amplitude        beta           r0           xpos
    ------------- ------------- ------------- --------------
    1.02990367011 1.12561141593 1.05475525089 0.996366721785
[ 0.02092153  0.28918073  0.20110149  0.01010909]

beta1d

@nden where should I add this fitting test?

@nden
Copy link
Contributor

nden commented Jul 20, 2017

@mirca If you add the model to tests/example_models.py it will be picked up by the fitting testers.

@mirca
Copy link
Member Author

mirca commented Jul 20, 2017

  • add change log entry
  • move implementation from functional_models.py to powerlaws.py
  • add Beta1D to tests/example_models.py

@mirca
Copy link
Member Author

mirca commented Jul 21, 2017

travis failures look unrelated?


See Also
--------
Lorentz1D
Copy link
Member

Choose a reason for hiding this comment

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

this causes the sphinx fail, use the full namespace as Lorentz1D is not defined in this file.

@bsipocz
Copy link
Member

bsipocz commented Jul 21, 2017

@mirca - The sphinx fail is real, and I've restarted the other one that timed out (it was probably unrelated, but we'll see).

@larrybradley
Copy link
Member

larrybradley commented Jul 21, 2017

Thanks, @mirca. The Beta1D model is simply a slight reparameterization of the existing Moffat1D model. Astropy used to have both Beta1Dand Beta2D models (from Sherpa ), but it was decided to change them to Moffat1D and Moffat2D. Previous discussions:

#3029

https://groups.google.com/d/msg/astropy-dev/vizuhH8I3PA/IuhNz6qrlX4J

@nden
Copy link
Contributor

nden commented Jul 22, 2017

Given all this is anyone opposed to adding a note to the Moffat classes to the effect that they are reparameterization of the Beta models in Sherpa?

@larrybradley Could you look over this list and see if it is up to date.

@mirca
Copy link
Member Author

mirca commented Jul 22, 2017

@larrybradley Oh ok, thanks for pointing that out!

@mirca mirca closed this Jul 22, 2017
@larrybradley
Copy link
Member

@nden shell2d looks to be the same as the already implemented Ring2D model (http://docs.astropy.org/en/stable/api/astropy.modeling.functional_models.Ring2D.html)

And there is now a PR to add Schechter1D (you can remove the closed doc PR reference).

Everything else looks up to date.

@mirca mirca deleted the add-beta1d-model branch July 24, 2017 16:23
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.

None yet

5 participants