Add model separability test #6746

nden · 2017-10-16T14:57:14Z

Resolves #6013.

astropy-bot · 2017-10-16T14:57:16Z

Hi there @nden 👋 - 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 labeled 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! 👍

If there are any issues with this message, please report them here.

bsipocz · 2017-10-16T15:14:07Z

(@nden - I've removed the "affects dev" label, as we supposed to use it for bugfixes that are only in the dev version, thus they don't need a changelog entry.
This one on the other hand seems to be an added feature, so the changelog entry is definitely required.

nden · 2017-10-16T15:58:48Z

@bsipocz Oh, right. Thanks.

pllim

PEP8 test failed: astropy/modeling/tests/test_separable.py:108:1: W391 blank line at end of file

Also, I think some public-facing documentation is needed. For example:

I want to add a new model, how do I know if my new model is separable or not?
Why do I care if the models are separable or not? What happens when my compound model is separable or not?

pllim · 2017-10-16T16:10:17Z

astropy/modeling/core.py

+            return self._separable
+        else:
+            raise NotImplementedError(
+            'The "separable" property is not defined for '


Text needs to be indented more. Not sure why PEP8 check didn't complain about this one. Is there a tab in there that I can't see here?

pllim · 2017-10-16T16:12:12Z

astropy/modeling/separable.py

+
+     Examples
+    --------
+    >>> is_separable(Shift(1) & Shift(2) | Scale(1) & Scale(2))


I have a feeling that this won't pass doctest due to missing imports.

I guess this is more of a question for @bsipocz -- How did this pass the doctest? I thought we test codes inside docstring, right? If so, is a docstring from hidden function (below) tested?

It seems that doctest has access all the imports of a given file, so has access to both is_separable as well as the models used here. I would still suggest to have full working examples in the example section, with doing all the necessary imports.

pllim · 2017-10-16T16:13:40Z

astropy/modeling/separable.py

+        An array of shape (transform.n_outputs,) of boolean type
+        Each element represents the separablity of the corresponding output.
+
+


Nitpick: Remove extra blank lines in docstring.

pllim

Left some more comments. Thank you for your patience.

pllim · 2017-11-06T16:09:42Z

docs/modeling/models.rst

+It indicates whether the outputs are independent and is essential for computing the
+separability of compound models using the :func:`~astropy.modeling.is_separable` function.
+Having a separable compound model means that it can be decomposed into independent models,
+whic in turn is useful in many applications.


whic -> which

pllim · 2017-11-06T16:10:44Z

docs/modeling/models.rst

+separability of compound models using the :func:`~astropy.modeling.is_separable` function.
+Having a separable compound model means that it can be decomposed into independent models,
+whic in turn is useful in many applications.
+Sometimes it is easier to define inverses using the independent parts than the


I am not sure how this sentence relates to "separability," please clarify.

I hope it's clearer now but let me know if not.

pllim · 2017-11-06T16:11:35Z

docs/modeling/new.rst

@@ -124,6 +124,10 @@ value of the `~astropy.modeling.Model.linear` attribute is ``False``.  Linear
 models should define the ``linear`` class attribute as ``True``.  Because this
 model is non-linear we can stick with the default.

+Models which inherit from `~astropy.modeling.Fittable1DModel` have the
+``Model._separable`` property set already to ``True``.
+Other models shoul define this property.


shoul -> should

pllim · 2017-11-06T16:12:13Z

docs/modeling/new.rst

@@ -124,6 +124,10 @@ value of the `~astropy.modeling.Model.linear` attribute is ``False``.  Linear
 models should define the ``linear`` class attribute as ``True``.  Because this
 model is non-linear we can stick with the default.

+Models which inherit from `~astropy.modeling.Fittable1DModel` have the
+``Model._separable`` property set already to ``True``.


Cross-ref to the separability section in models.rst.

pllim · 2017-11-06T16:14:59Z

astropy/modeling/tests/test_separable.py

+from astropy.modeling.models import Mapping
+import pytest
+import numpy as np
+from numpy.testing import utils


from numpy.testing import assert_allclose works for me. Is there a reason utils needs to be imported at all? I am afraid this might be confusing down the road because there is also utils in astropy itself.

pllim · 2017-11-06T16:15:45Z

astropy/modeling/tests/test_separable.py

+
+"""
+
+from astropy.modeling import models


Why is this using absolute import even though you have relative import below? Why not just have all astropy imports be relative imports here?

pllim · 2017-11-06T16:20:36Z

astropy/modeling/separable.py

+
+     Examples
+    --------
+    >>> is_separable(Shift(1) & Shift(2) | Scale(1) & Scale(2))


I guess this is more of a question for @bsipocz -- How did this pass the doctest? I thought we test codes inside docstring, right? If so, is a docstring from hidden function (below) tested?

bsipocz · 2017-11-07T15:50:49Z

astropy/modeling/separable.py

+
+from .core import Model, _CompoundModel, ModelDefinitionError
+from .mappings import Mapping
+from .models import Shift, Scale, Rotation2D, Polynomial2D


Looks like there are only used in examples in the docstrings. Would it be possible to rework those examples to contain all the necessary imports there, otherwise none of those would work as copy pasted for the users.

nden · 2017-11-08T14:12:12Z

@bsipocz , @pllim I've addressed your comments, hopefully things are clearer now. Thanks for looking over the PR.

pllim

Just one more nitpick. Otherwise LGTM. Thanks!

pllim · 2017-11-08T15:02:44Z

astropy/modeling/tests/test_separable.py

+"""
+
+from .. import models
+from ..models import Mapping


Nitpick: Group all the local imports together to make it easier for the future. (I am starting to sound like flake8 😱 )

astrofrog · 2017-11-10T09:58:41Z

CHANGES.rst

@@ -54,6 +54,9 @@ astropy.modeling

 - A ``deepcopy()`` method was added to models. [#6515]

+- Added ``is_separable`` function to modeling totest the


Also should this say 'separable property' instead of 'is_separable function'?

Now I am confused. I thought is_separable is a function that is defined in the new separable module?

It is true that this PR adds also Model.separable property which is only of interest when one writes a new model. The important functionality from a user point is the function is_separable. For completeness I added the separable property to the change log as well.

astrofrog · 2017-11-10T10:00:18Z

docs/modeling/models.rst

+than the entire model.
+In other cases, tools using `GWCS <https://gwcs.readthedocs.io/en/latest/>`_,
+can be more flexible and take advantage of separable spectral and spatial transforms.
+


Could you clarify/demonstrate how one would actually decompose a model?

@astrofrog The test tells whether the outputs are independent, it doesn't provide a mechanism for decomposing models. For decomposing you may need other information. This does as much as the example in the is_separable function.
Do you have a specific use case in mind? Or do you find this explanation misleading or unclear?

pllim · 2017-12-12T21:57:32Z

Need to rebase to resolve conflict.

@astrofrog , do you think this is okay to merge after rebase?

nden · 2017-12-14T13:38:07Z

rebased and squashed

pllim · 2017-12-14T17:11:58Z

astropy/modeling/separable.py

+    --------
+    >>> from astropy.modeling.models import Shift, Rotation2D
+    >>> _coord_matrix(Shift(1), 'left', 2)
+        array([[ 1.],


You need to unindent so doctest can pass. FYI.

Numpy dev and prerelease report arrays without a space before the values:
arrays([[1], vs array([[ 1], Any ideas how to get around this?

…ent numpy versions

nden · 2017-12-15T16:22:33Z

This is a well aged PR that I would like to get into v 3.0 to support further work on gwcs. I am planning to merge tonight, let me know if anyone needs more time to look at it.

nden added Affects-dev PRs and issues that do not impact an existing Astropy release Enhancement modeling Work in progress labels Oct 16, 2017

nden added this to the v3.0.0 milestone Oct 16, 2017

nden requested a review from pllim October 16, 2017 14:57

bsipocz removed the Affects-dev PRs and issues that do not impact an existing Astropy release label Oct 16, 2017

pllim reviewed Oct 16, 2017

View reviewed changes

nden force-pushed the separable branch from 64dadc3 to cb962d3 Compare October 24, 2017 16:26

nden added Ready-for-final-review and removed Work in progress labels Oct 27, 2017

pllim reviewed Nov 6, 2017

View reviewed changes

bsipocz reviewed Nov 7, 2017

View reviewed changes

nden changed the title ~~WIP: add separability test~~ Add model separability test Nov 8, 2017

pllim approved these changes Nov 8, 2017

View reviewed changes

astrofrog reviewed Nov 10, 2017

View reviewed changes

bsipocz mentioned this pull request Nov 21, 2017

Fix example in astropy.table.unique docstring #6878

Merged

pllim added this to Needs decision in 3.0 Feature Planning Dec 13, 2017

add separability test

6bba41f

nden force-pushed the separable branch from 97099e5 to 6bba41f Compare December 14, 2017 13:37

avoid printing dtype=np.bool in doctest

e2c3fec

pllim reviewed Dec 14, 2017

View reviewed changes

nden added 2 commits December 14, 2017 16:02

fix doctest

ba2f238

remove examples from docstrings because of different output in differ…

04768d2

…ent numpy versions

nden merged commit 9126c92 into astropy:master Dec 21, 2017

nden deleted the separable branch December 27, 2018 19:29

pllim mentioned this pull request May 13, 2021

Clarify definition of modeling.separable #11741

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add model separability test #6746

Add model separability test #6746

nden commented Oct 16, 2017

astropy-bot bot commented Oct 16, 2017 •

edited

bsipocz commented Oct 16, 2017

nden commented Oct 16, 2017

pllim left a comment

pllim Oct 16, 2017

pllim Oct 16, 2017

pllim Nov 6, 2017

bsipocz Nov 7, 2017

pllim Oct 16, 2017

pllim left a comment

pllim Nov 6, 2017

pllim Nov 6, 2017

nden Nov 8, 2017

pllim Nov 6, 2017

pllim Nov 6, 2017

pllim Nov 6, 2017

pllim Nov 6, 2017

pllim Nov 6, 2017

bsipocz Nov 7, 2017

nden commented Nov 8, 2017

pllim left a comment

pllim Nov 8, 2017

astrofrog Nov 10, 2017

astrofrog Nov 10, 2017

pllim Nov 10, 2017

nden Nov 14, 2017

astrofrog Nov 10, 2017

nden Nov 14, 2017

pllim commented Dec 12, 2017

nden commented Dec 14, 2017

pllim Dec 14, 2017

nden Dec 14, 2017

nden commented Dec 15, 2017

		An array of shape (transform.n_outputs,) of boolean type
		Each element represents the separablity of the corresponding output.

		@@ -54,6 +54,9 @@ astropy.modeling

		- A ``deepcopy()`` method was added to models. [#6515]

		- Added ``is_separable`` function to modeling totest the

Add model separability test #6746

Add model separability test #6746

Conversation

nden commented Oct 16, 2017

astropy-bot bot commented Oct 16, 2017 • edited

bsipocz commented Oct 16, 2017

nden commented Oct 16, 2017

pllim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pllim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nden commented Nov 8, 2017

pllim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pllim commented Dec 12, 2017

nden commented Dec 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nden commented Dec 15, 2017

astropy-bot bot commented Oct 16, 2017 •

edited