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 general API tests #20

Merged
merged 9 commits into from
Jul 13, 2016
Merged

Added general API tests #20

merged 9 commits into from
Jul 13, 2016

Conversation

astrofrog
Copy link
Member

Still trying to figure out how to turn this into a markdown table...

Table of doom:

Test PolygonPixel EllipsePixel RectanglePixel CirclePixel
pix_area
pix_in
pix_to_mask:all
pix_to_mask:any
pix_to_mask:center
pix_to_mask:exact
pix_to_shapely
pix_to_sky:affine
pix_to_sky:full
pix_to_sky:local
Test CircleSky RectangleSky EllipseSky PolygonSky
sky_area
sky_in
sky_to_pix:affine
sky_to_pix:full
sky_to_pix:local

@cdeil
Copy link
Member

cdeil commented Jun 23, 2016

@astrofrog - Can you please rebase this to trigger tests to run again.

Also -- what's the plan here?
Can we merge this now / soon or is this some long-term WIP PR?

@cdeil
Copy link
Member

cdeil commented Jul 7, 2016

@astrofrog Do you have time to finish up this PR in the next two days for the 0.1 release? Or should we punt this one to the 0.2 milestone?

@astrofrog
Copy link
Member Author

@cdeil - I've just rebased and now made sure that methods that return NotImplementedError are xfailed for now.

Note that there are some genuine failures, either due to my lack of understanding of the API for certain tests, or real issues with different methods. I don't have time to look at this in the short term, but if someone else could take a look and do a PR to my branch to fix the real failures, then I can merge that in and we can make sure this makes it to 0.1. Some of the failures I saw are definitely of the kind that we should investigate before any release.

@astrofrog astrofrog mentioned this pull request Jul 7, 2016
@cdeil cdeil self-assigned this Jul 7, 2016
@cdeil
Copy link
Member

cdeil commented Jul 7, 2016

@astrofrog - I agree it would be very good to get this in for 0.1. I'll have a look at the fails now and try to fix them.

@cdeil
Copy link
Member

cdeil commented Jul 12, 2016

@astrofrog - Locally this makes all tests pass for me:
astrofrog#1

I guess the main question is if / how PixCoord should support array values and indexing, but that should be a separate issue / PR?

One more question: what about the "Table of doom" at the top? Does it make sense to copy it to a separate issue?
Probably we want some kind of such overview table concerning the implementation status for certain regions, right?
But maintaining it as a Markdown table is extra work.
Maybe move it to the wiki where everyone can update it and make a reminder issue?

@astrofrog
Copy link
Member Author

I guess the main question is if / how PixCoord should support array values and indexing, but that should be a separate issue / PR?

IMHO it should do what SkyCoord does behavior wise? (that is, it should be able to represent scalar and vector coordinates). I can't remember if SkyCoord actually stores everything as a numpy array though, I think it does.

@astrofrog
Copy link
Member Author

I agree moving the table to the wiki makes sense - can try and do that later today

Misc fixes to make test_api pass
@cdeil
Copy link
Member

cdeil commented Jul 12, 2016

travis-ci tests passed.
👍 to merge.

@@ -13,7 +13,7 @@ environment:
# For this package-template, we include examples of Cython modules,
# so Cython is required for testing. If your package does not include
# Cython code, you can set CONDA_DEPENDENCIES=''
CONDA_DEPENDENCIES: "Cython"
CONDA_DEPENDENCIES: "Cython shapely"
Copy link
Member

Choose a reason for hiding this comment

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

The Appveyor build fails because shapely is not available in this conda channel:
https://ci.appveyor.com/project/Astropy/regions/build/1.0.74/job/wfb95tev3c94p9jh#L81

@astrofrog - Do you know how to fix this? If no, please remove shapely here and let's move the Windows shapely install issue to a separate future issue.

@cdeil
Copy link
Member

cdeil commented Jul 13, 2016

@astrofrog - Could you please merge this PR soon?

I'd like to do some more PRs on regions and then do the 0.1 release very soon.
We need it for Gammapy, where we have removed gammapy.regions and are now using the circle regions from this package, i.e. it's a dependency I'd like to have available before making the Gammapy 0.5 release.

@astrofrog
Copy link
Member Author

@cdeil - I'm overloaded today and tomorrow - since you have merge privileges, can you merge this and make the fixes you need to make in a separate PR?

@astrofrog
Copy link
Member Author

@cdeil - actually let me push a fix for shapely, can do it now

@cdeil
Copy link
Member

cdeil commented Jul 13, 2016

Merging this now. Thanks, @astrofrog !

I'll continue with regions tomorrow.

@cdeil cdeil merged commit 37c5ca0 into astropy:master Jul 13, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants