Fixes for weights with linear fitter by saimn · Pull Request #10687 · astropy/astropy

saimn · 2020-08-28T15:39:08Z

Fix #7035.
I still find the modeling code quite confusing between the data axes, model sets, parameters, so I hope that I did it right for all cases (and tests seem to show that it works).

jehturner · 2020-09-02T18:04:49Z

Thanks, @saimn! I may be wrong, but I think this can be simplified significantly. Let me check...

jehturner · 2020-09-02T23:12:26Z

No, that idea was just a misunderstanding of how you've done it and why -- but could we maybe combine the separate loops for masked & weights? I could work on that if you want (I also had one or two remaining uncertainties about the 2D weights behaviour that I could check at the same time).

I think it's good to have several tests where the weights aren't all 1, to make sure the results are correct and not just that the code runs. I see you do have one test that uses real weights for a single 2D model and also one for a 1D model set, which covers the main cases of looping & not looping. I wonder about adding some real weights for a 2D model set, to make sure the algebra is still correct in that case, but this is already pretty good and it's great that you're covering all these cases.

saimn · 2020-09-03T08:53:26Z

could we maybe combine the separate loops for masked & weights? I could work on that if you want

Sure, another pair of eyes would be useful ;).

I think it's good to have several tests where the weights aren't all 1

Indeed. I needed tests to cover quickly all the different kind of inputs, so I basically copied the existing tests and added weights, but using something else than 1 requires to spend more time understanding each test case. We should certainly do that, if the algorithmic part is on the good path.

jehturner

I haven't actually finished reviewing this but I'll do "finish your review" because I think you won't see my couple of line comments otherwise? How do you want any changes I make? I would think a PR on your PR (like Curtis's) is best, since they'll probably be too complicated for github suggestions. At least this should be easier now we can check further changes against your tests...

saimn · 2020-09-03T15:57:54Z

@jehturner - Yes, a PR on my PR would work 👍

pllim · 2020-09-03T16:17:53Z

PR on my PR

Random thought: One of these days, we need to see how many PR to PR we can chain for a single PR. 😄

jehturner · 2020-09-03T16:19:19Z

Maybe we could try a circular one?

jehturner · 2020-09-05T01:43:11Z

+    def test_linear_fit_2d_model_set_common_weight(self):
+        init_model = models.Polynomial2D(degree=2, c1_0=[1, 2], c0_1=[-0.5, 1],
+                                         n_models=2,
+                                         fixed={'c1_0': True, 'c0_1': True})


Just wondering whether there's any special reason for fixing parameters here and in the other test?

nden · 2020-09-08T15:44:22Z

Is this ready for review?
@jehturner You are most familiar with this code, I'll wait for your approval before I review it.

jehturner · 2020-09-08T21:44:44Z

I think the code is correct now, @nden, ~~except that I was still checking what happens with scl~~. It would be good if the separate loops for weights.ndim > 1 and len(model_copy) > 1 and masked could be consolidated in a reasonably efficient way though; I was hoping to look at that last week but didn't get to it. I think @saimn and I are both out next week and am not sure if one of us will get to it before then (or convince ourselves it would be too messy).

saimn · 2020-09-11T11:53:41Z

@nden @perrygreenfield - This is now ready for review, thanks to @jehturner for addressing the remaining items :)

pllim

Any plans to address codecov comment about certain LOCs not covered? If it is not covered on purpose, can add # pragma: no cover directive too.

Need another rebase for the change log conflict.

p.s. When this is merge, the weight is off your shoulder! 😉

pllim · 2020-09-11T14:58:30Z

+        assert_allclose(model.c1_0, -0.1, atol=1e-12)
+        assert_allclose(model.c0_1, 0.2, atol=1e-12)
+
+    # # The following 2 cases also happen to work, but model set fitting is


Why not use pytest.mark.skip?

Not sure if we need to keep those tests, @jehturner ?

I don't think we particularly need to keep them, but if they're there they could just be uncommented when those fitters support model sets. They do also work now, but I'm not sure it's reasonable to require those cases to continue working as long as the fitters aren't advertised to support model sets (so there's no reason for there to be an extra dimension).

saimn · 2020-10-08T18:48:47Z

We (with @jehturner) are working on integrating this in our fitting code, it works well, and I just found a little issue when weights for one of the models are all 0 and the data is not a masked array. In that case the lapack routine in Numpy will crash with a rather obscure exception (ValueError: On entry to DLASCL parameter number 4 had an illegal value) so I added a check for this to raise a better exception. If the data is a masked array there will be a RuntimeWarning: invalid value encountered in true_divide but the code does not crash and just return a model with NaN parameters.

ping @nden @perrygreenfield - please review when you can :)

…d inputs.

…& y. Only flatten weights completely when common to multiple models. Check consistency of array shapes in _convert_input for all cases: not just the model_set_axis for 1D model sets and the x/y/z shapes for 2D model sets.

Avoid some extra transpositions.

This is to raise a better error than the Numpy one which is not so clear: ValueError: On entry to DLASCL parameter number 4 had an illegal value

saimn · 2020-10-26T16:20:52Z

@nden - I moved most of the model set tests to test_model_sets.py, but there were already some model set tests in test_fitting.py, and for some of the new model set tests we use the fixtures/class from test_fitting.py.
I also squashed some commits (e.g. changelog related), but I'm not sure what you want me to do here ? Do you want a single commit for this whole blob of changes ? IMO some of them deserve their own commit to be understandable (if one has to browse the history in the future) and some have comments that could be helpful as well...

saimn · 2020-10-29T17:02:43Z

Ping @nden, would be super useful for us to have this in the next bugfix releases (i.e. 4.1.1).

nden · 2020-10-30T17:28:55Z

@pllim Is readthedocs failure a known issue, they seem unrelated? Please feel free to merge of so.

pllim

RTD might be online again but given the diff, it is not critical for merging even if server still down.

pllim · 2020-10-30T17:41:58Z

+        assert_allclose(model.c1_0, -0.1, atol=1e-12)
+        assert_allclose(model.c0_1, 0.2, atol=1e-12)
+
+    # # The following 2 cases also happen to work, but model set fitting is


I am not sure about this one. Can we use pytest.mark.skip instead?

I talked with @jehturner and I think it's better to remove them, since this is not a supported feature. If we can keep them in the history, maybe at some point we can find them again if it's useful.

I pushed a commit to remove these tests, and skipped CI since this is removing only commented lines.

pllim · 2020-10-30T19:22:44Z

RTD warnings are real but unrelated. Merging then. Thanks, everyone!

Fixes for weights with linear fitter

saimn added the modeling label Aug 28, 2020

saimn added this to the v4.0.2 milestone Aug 28, 2020

saimn requested review from jehturner and nden August 28, 2020 15:39

saimn force-pushed the linear-fitter-weights branch 2 times, most recently from 103f69d to cb304ef Compare August 28, 2020 15:42

pllim added the Bug label Aug 28, 2020

nden requested a review from perrygreenfield August 29, 2020 16:55

saimn commented Sep 3, 2020

View reviewed changes

Comment thread astropy/modeling/fitting.py Resolved

jehturner reviewed Sep 3, 2020

View reviewed changes

Comment thread astropy/modeling/fitting.py Resolved

Comment thread astropy/modeling/fitting.py Resolved

jehturner reviewed Sep 4, 2020

View reviewed changes

Comment thread astropy/modeling/fitting.py Resolved

jehturner reviewed Sep 5, 2020

View reviewed changes

jehturner reviewed Sep 8, 2020

View reviewed changes

Comment thread astropy/modeling/fitting.py Resolved

jehturner reviewed Sep 9, 2020

View reviewed changes

Comment thread astropy/modeling/fitting.py Resolved

saimn force-pushed the linear-fitter-weights branch from add97d5 to 8a91e84 Compare September 11, 2020 10:36

pllim reviewed Sep 11, 2020

View reviewed changes

saimn force-pushed the linear-fitter-weights branch from 50ed6f3 to 2f3ffc0 Compare September 23, 2020 20:28

saimn mentioned this pull request Sep 23, 2020

add attributes for fitting uncertainties to models #10552

Merged

saimn force-pushed the linear-fitter-weights branch from 2f3ffc0 to 113f725 Compare October 5, 2020 17:17

astrofrog removed this from the v4.0.2 milestone Oct 10, 2020

saimn and others added 13 commits October 26, 2020 13:06

Improve tests with non-1 weights for the 1D case

41706a6

Fixes following review

1811d91

Add tests for linear fitting of 2D models with weights using flattene…

f3502fc

…d inputs.

Simplify condition

b13f4ab

Allow fitter(s) to work with data that have a length-1 model set axis.

bbb83da

Tests for linear fitter on data with a length-1 model set axis.

dda519b

Combine the loops for masked and separately-weighted model sets.

2967550

Avoid some extra transpositions.

Add test for some errors

de591fa

Add back line from astropy#10552 after rebase

f9fddf8

Raise a better error when weights are 0 and not using a masked array

ffc1fce

This is to raise a better error than the Numpy one which is not so clear: ValueError: On entry to DLASCL parameter number 4 had an illegal value

Add changelog entry

ca4573c

Move tests to test_model_sets

0c2c831

saimn force-pushed the linear-fitter-weights branch from 62774a0 to 0c2c831 Compare October 26, 2020 16:08

pllim mentioned this pull request Oct 28, 2020

IRAF tasks of interest to be replaced astropy/specreduce#40

Open

nden approved these changes Oct 30, 2020

View reviewed changes

pllim reviewed Oct 30, 2020

View reviewed changes

Remove model set tests with LevMarLSQFitter and weights [skip ci]

2e63dad

pllim merged commit 3bae6fa into astropy:master Oct 30, 2020

saimn deleted the linear-fitter-weights branch October 30, 2020 19:23

bsipocz pushed a commit that referenced this pull request Nov 6, 2020

Merge pull request #10687 from saimn/linear-fitter-weights

b4163c4

Fixes for weights with linear fitter

bsipocz pushed a commit that referenced this pull request Nov 6, 2020

Merge pull request #10687 from saimn/linear-fitter-weights

e535913

Fixes for weights with linear fitter

bsipocz pushed a commit that referenced this pull request Nov 7, 2020

Merge pull request #10687 from saimn/linear-fitter-weights

1f61cc4

Fixes for weights with linear fitter

saimn mentioned this pull request Nov 9, 2020

[4.2rc1] Test failures on armel: no FPEs #11011

Closed

astrofrog pushed a commit that referenced this pull request Nov 10, 2020

Merge pull request #10687 from saimn/linear-fitter-weights

ba03383

Fixes for weights with linear fitter

bsipocz pushed a commit that referenced this pull request Nov 23, 2020

Merge pull request #10687 from saimn/linear-fitter-weights

5ea6850

Fixes for weights with linear fitter

Conversation

saimn commented Aug 28, 2020

Uh oh!

jehturner commented Sep 2, 2020

Uh oh!

jehturner commented Sep 2, 2020

Uh oh!

saimn commented Sep 3, 2020

Uh oh!

Uh oh!

jehturner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

saimn commented Sep 3, 2020

Uh oh!

pllim commented Sep 3, 2020

Uh oh!

jehturner commented Sep 3, 2020

Uh oh!

Uh oh!

jehturner Sep 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nden commented Sep 8, 2020

Uh oh!

Uh oh!

jehturner commented Sep 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

saimn commented Sep 11, 2020

Uh oh!

pllim left a comment

Choose a reason for hiding this comment

Uh oh!

pllim Sep 11, 2020

Choose a reason for hiding this comment

Uh oh!

saimn Sep 23, 2020

Choose a reason for hiding this comment

Uh oh!

jehturner Sep 29, 2020

Choose a reason for hiding this comment

Uh oh!

saimn commented Oct 8, 2020

Uh oh!

saimn commented Oct 26, 2020

Uh oh!

saimn commented Oct 29, 2020

Uh oh!

nden commented Oct 30, 2020

Uh oh!

pllim left a comment

Choose a reason for hiding this comment

Uh oh!

pllim Oct 30, 2020

Choose a reason for hiding this comment

Uh oh!

saimn Oct 30, 2020

Choose a reason for hiding this comment

Uh oh!

saimn Oct 30, 2020

Choose a reason for hiding this comment

Uh oh!

pllim commented Oct 30, 2020

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jehturner Sep 5, 2020 •

edited

Loading

jehturner commented Sep 8, 2020 •

edited

Loading