Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added metadata test for GenericMap.resample #883

Merged
merged 4 commits into from
Mar 19, 2014

Conversation

mdmueller
Copy link
Contributor

As addressed in issue #760. I also made the other resample test a bit more concise.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 3fbf15b on amras1:genericmap-resample-testing into ee1d14f on sunpy:master.

@dpshelio
Copy link
Member

dpshelio commented Mar 4, 2014

PR Looks very nice! 👍

I'm thinking if we should just change size and coordinates on the header of a resample image, or if we should also add something in the history, so whoever finds a resample file later on knows it has been resampled.

resampled_map = self.map.resample(new_dimensions, method = sample_method)
assert resampled_map.shape[1] == new_dimensions[0]
assert resampled_map.shape[0] == new_dimensions[1]

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't it easier to tell which test failed under the old version?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking the same... but if it fails you get the value tested within the error message, so at least the dimensions will tell you which one was used. I think.

I'm not saying that's easier to tell though. But from a list of 4 is quite easy.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@derdon Can't we use some pytest magic here?

@mdmueller
Copy link
Contributor Author

Fixed the spaces around =, is there a convenient way to give more information about the assertion using pytest? I couldn't find anything except assert foo == bar, msg which loses introspection.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.53%) when pulling 1eb932a on amras1:genericmap-resample-testing into ee1d14f on sunpy:master.

@derdon
Copy link
Member

derdon commented Mar 5, 2014

You usually don't need a custom message because the output of py.test is quite helpful. Just use assert out == expted

@Cadair
Copy link
Member

Cadair commented Mar 10, 2014

Can't we use pytest parametrize to make this cleaner? https://pytest.org/latest/example/parametrize.html

@rhewett
Copy link
Member

rhewett commented Mar 10, 2014

Yes, this is how it should be done. Then, each one will count as a different test, too, which they should.

@mdmueller
Copy link
Contributor Author

Okay fixed, hopefully pytest will report enough info now.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 630d8d2 on amras1:genericmap-resample-testing into ee1d14f on sunpy:master.

@Cadair
Copy link
Member

Cadair commented Mar 18, 2014

👍 from me.

dpshelio added a commit that referenced this pull request Mar 19, 2014
Added metadata test for GenericMap.resample
@dpshelio dpshelio merged commit fb5b64d into sunpy:master Mar 19, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants