Enable unit support on all functional models #6183

astrofrog · 2017-06-12T08:10:59Z

The goal of this PR is to add tests for all functional models with units and fix any issues that come up. This is now ready for review. The only thing is that the absorption gaussian can't be made unit-aware because of the 1 - term - there is no way to attach a unit to 1 (I personally find this model strange since really one should do Const1D - Gaussian1D). Maybe we should replace 1 by a model parameter, but I suggest deferring that to a separate PR (I'm trying to keep my modeling PRs from getting out of control...)

bsipocz · 2017-06-13T13:00:52Z

@astrofrog - I have manually cancelled the last travis, sadly the skipping is not working as expected in PRs, but I'm on it.

pllim · 2017-06-13T15:34:14Z

@nden asked if you plan to address non-functional models, like projections, mapping, etc.

astrofrog · 2017-06-13T17:33:59Z

@pllim - I think projections are currently tested already thanks to @Cadair, but haven't looked into mappings yet. I do plan to take a look at this though, but not in this PR.

astrofrog · 2017-06-13T17:39:05Z

@nden @pllim - can you review this when you get a chance? Thanks!

pllim

I do have plans to fully test this functionality over at my synphot work but that won't happen before v2.0 release.

pllim · 2017-06-13T17:54:05Z

CHANGES.rst

-    model evaluation and fitting, added support for units on Gaussian1D
-    and added a new Blackbody1D model. [#4855]
+    model evaluation and fitting, added support for units on most
+    functional models and added a new Blackbody1D model. [#4855, #6183]


BlackBody1D (both B's are capitalized in the code).

pllim · 2017-06-13T17:56:08Z

astropy/modeling/core.py

@@ -1475,22 +1475,17 @@ def prepare_inputs(self, *inputs, **kwargs):
            for i in range(len(inputs)):

                input_name = self.inputs[i]
+                input_unit = self.input_units.get(input_name, None)


This change assumes self.input_units is now always a dictionary. The deleted code allowed for None type and a dictionary. Is this intentional?

At this point it's always a dict. The check for None is done earlier.

pllim · 2017-06-13T18:02:40Z

astropy/modeling/functional_models.py

-
-        return amplitude * np.sin(2 * np.pi * frequency * x + 2 * np.pi * phase)
+        argument = TWOPI * (frequency * x + phase)
+        if isinstance(argument, Quantity):


If it is not Quantity and you want to attach radian unit to it, I get it. But here, you are multiplying radian unit to it but only if it is a Quantity, which I do not get. What are you trying to do here?

I wonder about this too. The code works but I'm not sure I understand why it's done this way.

Ah yes - I admit it's a little confusing - the reason for this is because if frequency and x are quantities, argument will end up being dimensionless because TWOPI doesn't have a unit (even though it should be radians). But then np.sin(dimensionless) crashes.

Another option would be to do argument = argument.value which would work too. I might do that instead since that will work too, and will put a comment.

pllim · 2017-06-13T18:43:27Z

astropy/modeling/tests/test_functional_models_quantities.py

+except ImportError:
+    HAS_SCIPY = False
+
+# TODO: GaussianAbsorption1D doesn't work with units because the 1- part doesn't


Good question. GaussianAbsorption1D as it is now is only useful as a multiplicative factor to a continuum to create an absorption line. Otherwise, you can create absorption line without it by using continuum_model - Gaussian1D. (This model might had been written a long time ago before the math operators worked properly?) I wonder if GaussianAbsorption1D is used by anyone at all. Maybe deprecate it for future removal? That way, we can just say "this model is deprecated and does not support units".

Related reading: http://synphot.readthedocs.io/en/latest/synphot/spectrum.html#gaussian-absorption

Maybe we could first ask if anyone is using it on astropy-dev - another option is to make the continuum level a parameter.

I opened an issue to discuss this: #6195

pllim · 2017-06-13T18:49:18Z

astropy/modeling/tests/test_functional_models_quantities.py

+        z = np.exp(-x.value**2 - y.value**2) * model['evaluation'][0][2].unit
+        args = [x, y, z]
+    fitter = LevMarLSQFitter()
+    m_new = fitter(m, *args)


This simply tests that fitter does not crash but does not test whether the fitted values are actually correct (both values and units).

That is correct - I should at least make sure that the resulting models have units - however I think checking the fitted parameter values is beyond the scopes of these tests

Ok, I've now expanded that to make sure the resulting model has units and that they are compatible with the starting units.

pllim · 2017-06-13T18:52:22Z

astropy/modeling/tests/test_functional_models_quantities.py

+
+
+@pytest.mark.parametrize('model', MODELS)
+def test_models_bounding_box(model):


Why is there a need to test this as part of this PR? Bounding box seems unrelated here. If your goal is to test whether bounding box has the correct unit, that does not seem to be tested.

Because for some models, bounding_box crashed if units were present, so I had to fix those and wanted a regression test. I agree we could check the actual values, but the purpose here is first and foremost to check that it does anything at all.

I've updated this to now check for the actual accuracy of the bounding box values too

nden · 2017-06-13T20:34:02Z

@astrofrog This looks good to me.
I got one error in GWCS due to a rotation model initialized with a SkyCoord object.
If no one is working on rotations I can try to add units there.

astrofrog · 2017-06-13T21:22:54Z

@nden - if you'd like to work on rotations that would be great! Just ping me if you run into any issues.

astrofrog · 2017-06-13T21:49:00Z

@nden @pllim - thanks for the reviews! I think I've addressed all your concerns/comments. If the CI passes, i think this is good to go, but I'll let one of you merge if you are happy with it (let's wait for Travis etc to run first though).

…g box)

…lso don't use any negative values for independent variables since that causes issues for some of the models which are not well defined in those cases (for example when the independent variable is radius)

astrofrog · 2017-06-14T07:16:28Z

@nden - just for info I plan to look into power laws and polynomials (in a separate PR), but I'll leave rotations for you to take a look at. This is ready for a final review/merge.

pllim

Just nitpicks on the tests. Also, maybe a squash? (I am notorious for asking people to squash, so if you disagree on that, you don't have to.)

pllim · 2017-06-14T14:24:59Z

astropy/modeling/tests/test_functional_models_quantities.py

+        # values one by one.
+        for i in range(len(model['bounding_box'])):
+            bbox = m.bounding_box
+            print(bbox)


print function does not seem necessary. Is this leftover from debugging phase?

pllim · 2017-06-14T14:25:50Z

astropy/modeling/tests/test_functional_models_quantities.py

+SCIPY_MODELS = set([Sersic1D, Sersic2D, AiryDisk2D])
+
+@pytest.mark.parametrize('model', MODELS)
+def test_models_evaluatate_without_units(model):


evaluatate -> evaluate (multiple occurrences)

astrofrog · 2017-06-14T15:06:20Z

@pllim - I've implemented the fixes. In general I think we should only ask people to squash if they spend say 10 commits making trivial changes, or if they accidentally add then remove a large file, but for 600+ line changes, I don't think it's unreasonable to have a dozen commits.

pllim

LGTM. I'll add a "merge when CI pass" tag. @nden has until then to disagree. 😉

astrofrog added this to the v2.0.0 milestone Jun 12, 2017

pllim added Affects-dev PRs and issues that do not impact an existing Astropy release modeling Work in progress units labels Jun 12, 2017

taldcroft requested a review from nden June 12, 2017 14:20

pllim mentioned this pull request Jun 12, 2017

Use unit support in Astropy models spacetelescope/synphot_refactor#124

Open

astrofrog force-pushed the enable-units-all-models branch from ede09c5 to 267d948 Compare June 13, 2017 13:19

astrofrog force-pushed the enable-units-all-models branch 4 times, most recently from edeffbc to df8ef51 Compare June 13, 2017 17:32

astrofrog added the Ready-for-final-review label Jun 13, 2017

astrofrog changed the title ~~WIP: Enable unit support on all functional models~~ Enable unit support on all functional models Jun 13, 2017

astrofrog requested a review from pllim June 13, 2017 17:38

pllim removed the Work in progress label Jun 13, 2017

pllim reviewed Jun 13, 2017

View reviewed changes

astrofrog added 5 commits June 14, 2017 07:52

Added tests for evaluation of all 1D functional models with quantities

5efb257

Added test of bounding_box when quantities are present

74c89e1

Test 2D functional models with quantities (for evaluation and boundin…

38c849d

…g box)

Fix fitting for 1D functional models with units

d6ee801

Fix fitting tests for all functional models

8f5fdf7

astrofrog added 9 commits June 14, 2017 07:52

Add tests without units for all functional models

7397b26

Minor fixes to logic

b0caec6

Skip models that require scipy if scipy is not installed

41f993f

Updated changelog entry [ci skip]

c2adf61

Don't use all ones for dependent variable when testing fitting, and a…

e185bb2

…lso don't use any negative values for independent variables since that causes issues for some of the models which are not well defined in those cases (for example when the independent variable is radius)

Addressed comments by @nden and @pllim

dd03dec

Test bounding box values

fd9f35d

Make sure that fitted models retain units in parameters

be6b28d

Remove workaround for Ring2D width

d435d22

astrofrog force-pushed the enable-units-all-models branch from 874b438 to d435d22 Compare June 14, 2017 05:53

pllim reviewed Jun 14, 2017

View reviewed changes

Fix test function names

ebe4e3c

pllim approved these changes Jun 14, 2017

View reviewed changes

pllim added the 💤 merge-when-ci-passes Do not use: We have auto-merge option now. label Jun 14, 2017

pllim mentioned this pull request Jun 14, 2017

Impose positive stddev in Gaussian models for fitting #6019

Merged

astrofrog merged commit d561702 into astropy:master Jun 14, 2017

MSeifert04 removed the 💤 merge-when-ci-passes Do not use: We have auto-merge option now. label Oct 17, 2017

astrofrog deleted the enable-units-all-models branch November 14, 2018 15:24

dhomeier mentioned this pull request Nov 16, 2022

Deprecate the humlicek2 method for Voigt1D #14013

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable unit support on all functional models #6183

Enable unit support on all functional models #6183

astrofrog commented Jun 12, 2017 •

edited

Loading

bsipocz commented Jun 13, 2017

pllim commented Jun 13, 2017

astrofrog commented Jun 13, 2017 •

edited

Loading

astrofrog commented Jun 13, 2017

pllim left a comment

pllim Jun 13, 2017

pllim Jun 13, 2017

nden Jun 13, 2017

pllim Jun 13, 2017

nden Jun 13, 2017

astrofrog Jun 13, 2017

pllim Jun 13, 2017

astrofrog Jun 13, 2017

astrofrog Jun 13, 2017

pllim Jun 13, 2017

astrofrog Jun 13, 2017

astrofrog Jun 13, 2017

pllim Jun 13, 2017

astrofrog Jun 13, 2017

astrofrog Jun 13, 2017

nden commented Jun 13, 2017

astrofrog commented Jun 13, 2017

astrofrog commented Jun 13, 2017

astrofrog commented Jun 14, 2017 •

edited

Loading

pllim left a comment

pllim Jun 14, 2017

pllim Jun 14, 2017

astrofrog commented Jun 14, 2017

pllim left a comment



		@pytest.mark.parametrize('model', MODELS)
		def test_models_bounding_box(model):

Enable unit support on all functional models #6183

Enable unit support on all functional models #6183

Conversation

astrofrog commented Jun 12, 2017 • edited Loading

bsipocz commented Jun 13, 2017

pllim commented Jun 13, 2017

astrofrog commented Jun 13, 2017 • edited Loading

astrofrog commented Jun 13, 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

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 Jun 13, 2017

astrofrog commented Jun 13, 2017

astrofrog commented Jun 13, 2017

astrofrog commented Jun 14, 2017 • edited Loading

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

astrofrog commented Jun 14, 2017

pllim left a comment

Choose a reason for hiding this comment

astrofrog commented Jun 12, 2017 •

edited

Loading

astrofrog commented Jun 13, 2017 •

edited

Loading

astrofrog commented Jun 14, 2017 •

edited

Loading