First attempt at circle masking #17
Conversation
|
@keflavich - can you rebase this? |
keflavich
force-pushed
the
circle_pixel_mask
branch
from
2b89caa
to
4a92a74
Compare
|
@keflavich - I added Cython to the Travis and AppVeyor config. Can you now rebase this and remove the empty commit at the same time? |
keflavich
force-pushed
the
circle_pixel_mask
branch
from
4a92a74
to
d262a14
Compare
|
done |
| @@ -1 +1 @@ | |||
| Subproject commit d8f48901442e4056879c4249e73ecd7d04a28282 | |||
| Subproject commit d51f726673510586d4b3bd04fa4799fdab389aab | |||
There was a problem hiding this comment.
No... I rebased and apparently the branch didn't have the most up-to-date astropy-helpers. I'm not entirely clear how this happened.
|
@keflavich - rebase, please. |
keflavich
force-pushed
the
circle_pixel_mask
branch
from
cabe68e
to
66142cb
Compare
|
@keflavich Is this ready for review? Can you remove the update of
|
extension of astropy#10 that has to be merged first.
keflavich
force-pushed
the
circle_pixel_mask
branch
from
66142cb
to
5f293e9
Compare
|
@cdeil done. I think this is ready for review, and skimming quickly it looks like it's code copied from somewhere else... probably @astrofrog's though I don't know where from. |
|
@keflavich - It looks very familiar, so most probably it's from photutils :) |
|
Cython compile error: |
| @@ -1 +1 @@ | |||
| Subproject commit 3e044eb26395c8569e3e15bf821c6ae5eeea324a | |||
| Subproject commit d51f726673510586d4b3bd04fa4799fdab389aab | |||
There was a problem hiding this comment.
@keflavich - The astropy_helpers submodule update is still in this PR. Please remove it.
|
This PR doesn't contain any tests. It doesn't make sense to add so much code without tests. So @keflavich, unless you or photutils people like @bsipocz or @larrybradley have time to work on this in the next two days, maybe we should put this to the 0.2 milestone? |
|
I don't have much brainpower on that 1-2days timescale to do anything remotely complicated, but why not copy over the very minimalist test we have in photutils? |
|
I won't be able to look at this until next week at the earliest. |
|
I don't think we should rush the migration of code from So I'm moving this to the 0.2 milestone (which could happen in a few weeks if someone works on this or other features). |
|
I think this is superseded by #71 ? @astrofrog - Can you please have another look over the diff in the PR here, and if there's nothing to copy over to #71 close it? |
|
@cdeil - yes, I actually cherry-picked some commits from here, and this is superseded. |
Followup to #10. Merge that first