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

Add tests for ds9 region parser #7

Merged
merged 33 commits into from Mar 25, 2016
Merged

Conversation

joleroi
Copy link
Contributor

@joleroi joleroi commented Mar 24, 2016

This is a continuation of #5
Addressing the 'correctness test'

  • Add functionality to write regions to ds9
  • Manually compare to input files an check everything is ok
  • Copy ds9 output to reference file and use for future testing

@joleroi
Copy link
Contributor Author

joleroi commented Mar 24, 2016

@keflavich I added tests that read the fk5, galactic, and physical input files and creates reference files (checked by hand). I guess that is sufficient to start playing with the parsers. What do you think?

@keflavich
Copy link
Contributor

Yes, the testing structure looks good. Any other interesting failure points along the way?

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling d7d1ae0 on joleroi:region_tests into * on astropy:master*.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling d7d1ae0 on joleroi:region_tests into * on astropy:master*.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 9f9c3fd on joleroi:region_tests into * on astropy:master*.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 6c01e44 on joleroi:region_tests into * on astropy:master*.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 6c01e44 on joleroi:region_tests into * on astropy:master*.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling ae9e624 on joleroi:region_tests into * on astropy:master*.

@joleroi
Copy link
Contributor Author

joleroi commented Mar 25, 2016

@keflavich
The test failure seems related to older astropy versions (that didn't have ecliptic coordinates?), I can try to have a look but I guess it's more efficient if someone who knows astropy does that.

@keflavich
Copy link
Contributor

@joleroi Looks like you're right. We should ask @mwcraig about the astropy versions linked to older numpy versions on conda: they seem on the old side (1.0.3<->1.8, 1.0.4<->1.9).

However, the solution is to do a version check in the testing and use
@pytest.mark.xfail(astropy.version < 1.(something))
where the version strings should be wrapped in StrictVersion from distutils.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling f9883a3 on joleroi:region_tests into * on astropy:master*.

@joleroi
Copy link
Contributor Author

joleroi commented Mar 25, 2016

One test fixed -> 2 others fail 😁

Seem like StrictVersion does not like the astropy dev version strings ...

https://travis-ci.org/astropy/regions/jobs/118531539#L633

@keflavich
Copy link
Contributor

Ah, that's true... maybe we need LooseVersion or .strip(".dev") or something.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling ab7a4c4 on joleroi:region_tests into * on astropy:master*.

@joleroi
Copy link
Contributor Author

joleroi commented Mar 25, 2016

Checks pass

@cdeil
Copy link
Member

cdeil commented Mar 25, 2016

Would it be possible to merge this today so that people (specifically me) can start playing with this?
Further work can always be done in future PRs, no?

@keflavich
Copy link
Contributor

Yes, reviewing.

@astrofrog
Copy link
Member

👍

@astrofrog astrofrog merged commit 95532a6 into astropy:master Mar 25, 2016
@joleroi joleroi deleted the region_tests branch March 27, 2016 20:58
@cdeil cdeil added this to the 0.1 milestone Jun 17, 2019
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

5 participants