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 n_submodels shared method to compound models #5747

Merged
merged 5 commits into from
Feb 8, 2017
Merged

Conversation

mirca
Copy link
Member

@mirca mirca commented Jan 27, 2017

This PR adds a property shared method called n_submodels to _CompoundModel which provides a clearer way to get the number of components in a compound model.

Fix #4093

cc @pllim @nden

Test that CompoundModel.get_n_submodels() properly returns the number
of components.
"""
g2 = Gaussian1D() + Gaussian1D()
Copy link
Member

Choose a reason for hiding this comment

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

We should also test that non-compound model return 1, or is that non-applicable? @nden?

@@ -176,6 +176,12 @@ In a future version it may be possible to "freeze" a compound model, so that
from the user's perspective it is treated as a single model. However, as this
is the default behavior it is good to be aware of.

One is also able to get the number of components (also known as submodels) in
a compound model by using ``get_n_submodels`` method::
Copy link
Member

Choose a reason for hiding this comment

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

The mention of "method" and its name needs to be corrected to reflect the correct new property.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @pllim

@pllim pllim added this to the v2.0.0 milestone Jan 27, 2017
@pllim
Copy link
Member

pllim commented Jan 27, 2017

Also need a change log for v2.0 under new feature.

@mirca
Copy link
Member Author

mirca commented Jan 27, 2017

@pllim @nden what about having n_submodels as a shared method so that it can also work as, e.g., (Gaussian1D + Gaussian1D).n_submodels()?

@mirca mirca changed the title Add n_submodels property to compound models Add n_submodels shared method to compound models Feb 2, 2017
@pllim pllim requested a review from nden February 3, 2017 17:00
@pllim
Copy link
Member

pllim commented Feb 3, 2017

LGTM 👍

@pllim
Copy link
Member

pllim commented Feb 3, 2017

@mirca , I can't comment about shared method. Perhaps @nden can as she has plans for composite models from classes.

@nden
Copy link
Contributor

nden commented Feb 7, 2017

@mirca 👍 for using a sharedmethod. Should this work with single models to avoid checking if the model is a compound model?

@mirca
Copy link
Member Author

mirca commented Feb 7, 2017

@nden It makes a lot of sense to me that this should work with single models too :)

@mirca
Copy link
Member Author

mirca commented Feb 7, 2017

@nden Let me know if the changes suffice, thanks!

Return the number of components in a single model, which is
obviously 1.
"""

Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: Blank line not necessary.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @pllim

@pllim
Copy link
Member

pllim commented Feb 8, 2017

LGTM 👍

@nden
Copy link
Contributor

nden commented Feb 8, 2017

Looks good! Thanks @mirca!

@nden nden merged commit 9c15fd9 into astropy:master Feb 8, 2017
@mirca mirca deleted the fix4093 branch February 8, 2017 21:25
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.

3 participants