WIP: Add more unit tests for modeling by cdeil · Pull Request #1060 · astropy/astropy

cdeil · 2013-05-07T08:32:45Z

I will add more unit tests for modelling in the coming weeks.

Further suggestions welcome.

To find the un-tested parts of modeling I use this command to generate the coverage report locally:
python setup.py test --coverage
There's also a nightly generated coverage report on the web:
https://jenkins.shiningpanda.com/astropy/job/astropy-coverage/coveragepy/?

cdeil · 2013-05-07T08:35:23Z

@nden Is this a bug or am I using MatrixRotation incorrectly?

In [1]: from astropy.models.rotations import MatrixRotation2D In [2]: m = MatrixRotation2D(angle=42) ERROR: AssertionError: Expected angle to be a number [astropy.models.rotations] --------------------------------------------------------------------------- AssertionError Traceback (most recent call last) <ipython-input-2-77a226a12d7e> in <module>() ----> 1 m = MatrixRotation2D(angle=42) /Users/deil/Library/Python/2.7/lib/python/site-packages/astropy-0.3.dev3922-py2.7-macosx-10.8-x86_64.egg/astropy/models/rotations.pyc in __init__(self, rotmat, angle) 173 outdim=1, paramdim=1) 174 else: --> 175 self._validate_angle(angle) 176 self._angle = Parameter('angle', np.deg2rad(angle), self, 1) 177 super(MatrixRotation2D, self).__init__(parnames=[], ndim=1, /Users/deil/Library/Python/2.7/lib/python/site-packages/astropy-0.3.dev3922-py2.7-macosx-10.8-x86_64.egg/astropy/models/rotations.pyc in _validate_angle(self, angle) 199 a = np.asarray(angle) 200 assert isinstance(a, numbers.Number), \ --> 201 "Expected angle to be a number" 202 assert a.ndim == 0, \ 203 "Expected angle to be a number" AssertionError: Expected angle to be a number

cdeil · 2013-05-07T19:31:59Z

I wanted to cover the CYP, CEA, CAR, MER and COP projections in the tests and to avoid code duplication parameterized test_projections.py in 7866fd0 .
But it doesn't work, when you uncomment these tests they in some cases raise an error for the given inputs or give non-matching results.

@nden Do you have a suggestion how I should change the test to cover those projections?

cdeil · 2013-05-10T14:00:32Z

@nden Can you add a simple example that shows the purpose of PCompositeModel to it's docstring, similar to the one in SCompositeModel here?

Note to self: Remaining things not covered by tests in core.py:

SCompositeModel.invert and PCompositeModel.invert
len(data) > 1 input to PCompositeModel.__call__.
PCompositeModel.add_model

(I really don't get the purpose of PCompositeModel so far.)

cdeil · 2013-05-10T16:41:52Z

This gives an error on Python 2.6: https://travis-ci.org/astropy/astropy/jobs/7057516#L1286
Does anyone know how to do this correctly?
(If not I'll simply remove that assert statement, it's not very important.)

Instead of exc.value try exc[1].

nden · 2013-05-10T20:26:06Z

@cdeil I don't think the cylindrical projections can be tested this way. The reason is that the cylindrical projections need parameters which are saved in the PV keywords and ~astropy.wcs does not expose programatically access to 'PV' parameters. The only way to test them is by using fits files. The fits files are already in https://github.com/astropy/astropy/tree/master/astropy/wcs/tests/maps.

Actually looking at wcs more carefully now, there might be a way to read and set the PV keywords but I can't get it to work.

@mdboom Should this work:

hdr_string = open('wcs/tests/maps/1904_66_CYP.hdr').read()
w = wcs.WCS(hdr_string)
w.wcs.get_pv()

The header has "PV" keywords but I am getting an empty list. Should I get a list of those? Further more if I do

w.wcs.set_pv([(2,1,1), (2,2,.7)])
w.wcs.p2s(np.array([[-10,30]]),1)

I get the same result as when I have not set the pv values.
Am I doing something wrong or are those not propagating to wcslib?

nden · 2013-05-10T21:33:55Z

@mdboom I think there was a problem with the data file. I can confirm now that get_pv() works.

cdeil · 2013-05-13T19:48:51Z

@nden @iguananaut What do you think? Should I assert the current string format here or remove the TODO?

The current is fine if you want to put in a test for this. Any changes I've proposed to the existing format are not special and can easily be changed. In fact if I did change the format there was no particular deliberate reason to do so. Maybe I just did whatever was useful to me for testing.

cdeil · 2013-05-13T20:02:11Z

@nden @iguananaut Maybe we can merge this PR soon and then we do separate issues / PRs for the remaining tasks mentioned here?

I can work on this tomorrow a bit though, so if there's anything you'd like me to add / change / fix in this PR, let me know.

cdeil · 2013-05-13T20:06:23Z

@nden @mdboom I have a basic questions about the models in projections.py: why do they re-implement the formulae in the __call__ methods? Wouldn't it be simpler / faster to use wcslib via astropy.wcs?

nden · 2013-05-14T01:51:14Z

@cdeil I've been working on a generalized WCS and the projections were implemented for that purpose. It is not clear to me whether we will use the current wcs in the future but even if we do my understanding is it is going to handle FITS WCS only. In any case the projections were a good test for the framework and I don't see a reason to remove them. I've adapted your tests to work with the maps in astropy.wcs as an illustration how they can be tested, feel free to add them to your tests.

nden@3ec7ec6

There's also a separate branch with minor fixes to the projections module and I ran the tests against it. For this I am going to make a pull request but not until I look at @iguananaut 's changes.

Fix projections Addresses some of the questions in #1060.

…y#1064

Address problems with constraints reported in #1060 and #1064 There's still a minor issue with constraints which is that after fitting the constraint attributes on the parameter objects are reset to the default values. But the model.constraints object has the correct values. I am going to merge this so it can be used and will open a different issue for this last problem I am aware of. It doesn't affect fitting but can be misleading.

…y#1064

embray · 2013-05-22T14:07:17Z

@cdeil Are you still planning to do more with this? If not, do you think you could rebase it against master so that I can merge it? These tests will be helpful in continuing to work on testing new ideas for the API.

eteq · 2013-05-29T05:55:49Z

Or is the issue that some of the TODOs need addressing by people other than @cdeil ?

nden · 2013-06-11T20:34:01Z

@cdeil Are you waiting for me to do something? Can this be merged?

eteq · 2013-06-18T16:44:29Z

looks like it needs to be rebased now...

cdeil · 2013-08-07T18:34:30Z

Sorry I started this big PR and then didn't have time to finish it.

A lot has changed in astropy.modeling in the meantime ... I'll probably compare by hand if there are useful parts in this PR that are not yet in master and then make a smaller new PR in the next days.

embray · 2013-08-08T14:35:31Z

👍 A lot of these tests would still be very useful to have. At one point I was delaying further work on #1086 until this was merged :)

nden · 2013-10-10T20:42:04Z

What's relevant in this PR was moved to #1548. Closing this.

cdeil reviewed May 7, 2013
View reviewed changes

cdeil mentioned this pull request May 7, 2013

Error creating MatrixRotation2D model #1068

Closed

cdeil mentioned this pull request May 7, 2013

Improve Polynomial models #1053

Closed

7 tasks

cdeil added 14 commits May 9, 2013 23:13

Add unit tests for models.rotations

52b5036

Minor misc cleanup in models

1bf869b

Move test_rotations.py from models to modeling

3e0530f

Add unit tests for modeling.utils

cc74b70

Make pmapdomain work correctly with list input oldx

cc4f983

Add more unit tests for modeling.rotations

492d2e2

Remove explicit ascii encoding for wcs.ctype in test_projections

1c55667

Fix typo in docstring

58f0edb

Parameterize astropy.modeling.projections tests

60078b4

Minor cleanup in modeling package

6123958

Add test_polynomial.py

6e2eab2

add missing deg2rad in MatrixRotation2D model

e0e6abe

Update modeling tests to latest changes

326d323

Minor whitespace changes

00f56fb

cdeil mentioned this pull request May 10, 2013

Make creating new simple models more user-friendly #980

Closed

cdeil added 6 commits May 10, 2013 11:10

Add more model parameter tests

2864599

Add test_functional_models.py

3a4c5a5

Rename test_models.py to test_core.py

a4f6044

Remove import * in modeling test_core.py

b6541d6

Fix SCompositeModel example and make it clearer

810b7fa

Add more tests for modeling.core classes

222d1cd

cdeil added 4 commits May 10, 2013 17:31

Adapt SCompositeModel example to doc standard

13d47ae

Adapt modeling constraints example to recent changes

ec577b0

Add unit test for constraints example

5ed05af

Remove import * in modeling

ca60aa3

cdeil added 2 commits May 10, 2013 18:28

Minor fix to modeling Constraints string formatting

4809a50

Add unit test for modeling Constraints.__str__

061f36e

cdeil reviewed May 10, 2013
View reviewed changes

embray mentioned this pull request May 13, 2013

New parameter handling for models, and other changes #1086

Merged

Fix test_core.py for Python 2.6 (remove unimportant assert)

2439761

cdeil reviewed May 13, 2013
View reviewed changes

nden added a commit that referenced this pull request May 14, 2013

Merge pull request #1092 from nden/fix_projections

f1c94fd

Fix projections Addresses some of the questions in #1060.

nden added a commit to nden/astropy that referenced this pull request May 16, 2013

Address problems with constraints reported in astropy#1060 and astrop…

bfae565

…y#1064

nden added a commit to nden/astropy that referenced this pull request May 20, 2013

Address problems with constraints reported in astropy#1060 and astrop…

c42998e

…y#1064

nden mentioned this pull request Oct 9, 2013

tests from 1060 #1548

Merged

nden added a commit to nden/astropy that referenced this pull request Oct 9, 2013

two more tests from astropy#1060

c18cd28

nden closed this Oct 10, 2013

Uh oh!

Uh oh!

Conversation

cdeil commented May 7, 2013

Uh oh!

cdeil May 7, 2013

Choose a reason for hiding this comment

Uh oh!

cdeil commented May 7, 2013

Uh oh!

cdeil commented May 10, 2013

Uh oh!

cdeil May 10, 2013

Choose a reason for hiding this comment

Uh oh!

embray May 10, 2013

Choose a reason for hiding this comment

Uh oh!

nden commented May 10, 2013

Uh oh!

nden commented May 10, 2013

Uh oh!

cdeil May 13, 2013

Choose a reason for hiding this comment

Uh oh!

embray May 13, 2013

Choose a reason for hiding this comment

Uh oh!

cdeil commented May 13, 2013

Uh oh!

cdeil commented May 13, 2013

Uh oh!

nden commented May 14, 2013

Uh oh!

embray commented May 22, 2013

Uh oh!

eteq commented May 29, 2013

Uh oh!

nden commented Jun 11, 2013

Uh oh!

eteq commented Jun 18, 2013

Uh oh!

cdeil commented Aug 7, 2013

Uh oh!

embray commented Aug 8, 2013

Uh oh!

nden commented Oct 10, 2013

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants