Skip to content

Add n_submodels shared method to compound models - #5747

Merged
nden merged 5 commits into
astropy:masterfrom
mirca:fix4093
Feb 8, 2017
Merged

nden merged 5 commits into
astropy:masterfrom
mirca:fix4093

Conversation

@mirca

@mirca mirca commented Jan 27, 2017 •

Copy link
Copy Markdown
Member

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
Copy Markdown
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?

Comment thread docs/modeling/compound-models.rst Outdated
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
Copy Markdown
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
Copy Markdown
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

pllim commented Jan 27, 2017

Copy link
Copy Markdown
Member

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

@mirca

mirca commented Jan 27, 2017

Copy link
Copy Markdown
Member Author

@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

pllim commented Feb 3, 2017

Copy link
Copy Markdown
Member

LGTM 👍

@pllim

pllim commented Feb 3, 2017

Copy link
Copy Markdown
Member

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

@nden

nden commented Feb 7, 2017

Copy link
Copy Markdown
Contributor

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

@mirca

mirca commented Feb 7, 2017

Copy link
Copy Markdown
Member Author

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

@mirca

mirca commented Feb 7, 2017

Copy link
Copy Markdown
Member Author

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

Comment thread astropy/modeling/core.py Outdated
Return the number of components in a single model, which is
obviously 1.
"""

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.

Nitpick: Blank line not necessary.

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.

Thanks @pllim

@pllim

pllim commented Feb 8, 2017

Copy link
Copy Markdown
Member

LGTM 👍

@nden

nden commented Feb 8, 2017

Copy link
Copy Markdown
Contributor

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