Merge imageutils functionality into astropy core by astrofrog · Pull Request #3201 · astropy/astropy

astrofrog · 2014-12-14T14:24:40Z

This is a WIP, but is the PR to merge imageutils into the Astropy core package.

Notes:

I have attempted to preserve the history but removed code that simply affected affiliated package infrastructure code.
The branches and merge history within imageutils is lost since I rebased this, but I think this is actually less confusing this way. We just have a linear history for the package.
I also updated issue numbers in the commit messages to always include astropy/imageutils
This does not include lacosmicx which can be added separately once the licensing is sorted out. Doing that separately will make reviewing this PR easier anyway
It is marked as critical because this was assessed as 1.0-critical (not a critical bug though)

This is not ready for review or merging, and still needs updates to the docs, tests to pass, etc.

cc @larrybradley @cdeil @adonath @eteq @hamogu @ejeschke

astrofrog · 2014-12-15T09:13:19Z

@cdeil @eteq - given that the normalization stuff applies to arrays of any dimension, should it be its own top-level package? What about other parts of this PR?

Here are remaining to-dos for this PR:

decide of final location of the features here (e.g. should normalization be a top-level package, and is anything else more general than just image?)
make sure all code here is documented, I think only the normalization stuff is properly documented
make sure test coverage is adequate

embray · 2014-12-15T20:23:55Z

Maybe this function could have a more specific name, like get_overlap or get_overlap_slices? (or just overlap_slices, where the get_ is implied?)

embray · 2014-12-15T20:36:51Z

I realize this is a WIP, but adding some comments anyways.... :D

astrofrog · 2014-12-15T20:39:48Z

@embray - thanks, this is helpful! The contents are actually fine for review, it's more questions to do with organization and docs that are WIP :)

embray · 2014-12-15T20:43:21Z

I guess this should add an abstractmethod for get_limits, or instead make an abstractproperty called just limits.

I'd like to leave get_limits because it is something that is computed on-the-fly and may take a non-trivial time in some cases, but happy to change if more people are in favor of a property.

Yeah, I don't think that's really the way to think of properties, IMO. This is debatable though.

On second thought, I misinterpreted what this is doing, and that it could return values dependent on the data. In that case this is fine.

mhvk · 2014-12-15T21:20:30Z

Just saw this now: I do agree that the display stuff is not specific to images and thus should be in a separate module. Since it is somewhat specifc to matplotlib, is it perhaps time to make a display or plot submodule? For the resampling, that really is not all that specific to images either (one could resample 1-d spectra). Doesn't it make more sense as part of nddata?

Bit higher-level: I was not part of the telecon, but I don't understand the rush to add this as a new package. Do we really want to offer long-term support for something this new? It seems ideal to start working on this after 1.0 is released.

astrofrog · 2014-12-15T21:28:33Z

@mhvk - sorry, I should clarify, most of the normalization code is matplotlib-agnostic, and can be used with n-dimensional arrays. It is true that in practice it will be used with images though.

Also, we aren't calling all features in 1.0 "LTS" so it's fine to put this in 1.0. Several packages are depending on this functionality, so it's good to get it in the core and it will mature very fast there. A lot of this is intended for affiliated packages, rather than to be used directly by users.

astrofrog · 2014-12-15T21:30:02Z

Another thing to bear in mind in the discussion of where this should all go. convolution used to be in astropy.nddata.convolution and we decided to give it more visibility by moving it to the top-level. I'm not saying everything should be in the top level, but just something to bear in mind.

embray · 2014-12-15T21:37:43Z

There's no reason to put raise NotImplementedError in an abstractmethod (since it won't be called in the first place if it's not implemented). Better is to just put a docstring and leave the rest empty (in fact, subclasses can still, in principle, call super().get_limits(), which in this case would result in raising NotImplementedError even though it is implemented).

astrofrog · 2014-12-16T08:42:05Z

Thinking about this a bit more, maybe normalization does belong where it is here - most of the time it's going to be used for images. Some of the array/index utilities could indeed be moved to e.g. astropy.utils.arrays though.

@larrybradley @cdeil @adonath @eteq @hamogu - what do you think?

@mhvk - just to clarify about your comment on starting to work on this after 1.0, I should say that this has already been worked on for a few months outside of the core, so it's not like it's all untested code. The difficult part is just figuring out where it should go in the core.

If we don't reach an agreement on where things should go in the core, then we have two options:

Merge it in as astropy._image so that a few affiliated packages can still start using it from there
Defer to 1.1

cdeil · 2014-12-16T10:32:55Z

I would prefer to keep array utils in astropy.image or astropy.nddata and not start another namespace astropy.utils.arrays. Arrays are images and thus it's OK to have array utils functions in astropy.image.

I'm also +1 to get this into astropy now ... for the reasons that @astrofrog said: these functions are used (or will be as soon as they land) by several affiliated packages (photutils, ccdproc, gammapy, ...) and the name imageutils is taken on PyPI, so we'd have to release under a different name than the repo name and ... we create extra maintenance work for ourselves.
But @mhvk, your points are very valid ... we will have some pain in the coming months if we want to change something in astropy.image that would break code in the affiliated packages. If absolutely needed they can keep bundling a stable copy of imageutils in extern like photutils is doing at the moment.

mhvk · 2014-12-16T14:18:16Z

Agreed that we don't need a new namespace for the array manipulation routines. I think those are not 2-D specific (even though many are written as such), and hence in my opinion are more natural in nddata as low-level n-D array manipulation tools. But it depends a little what in the end image is meant to be: is it mostly for things like mosaicing, drizzling, etc.?

The stretch and normalization routines are 2-D specific, but even more they are for displaying arrays, not for manipulating them, and the classes are also seem specific to matplotlib. It does seem that so far in astropy we have separated analysis from display (to the extent we hardly have anything for display!) and I quite like that. I think I'd rather have a new display namespace.

cdeil · 2014-12-18T23:27:31Z

Hey ... it takes data and mask ... why not @supports_nddata?
(I'm not seriously suggesting to do this by tomorrow ... but there's a few functions in this PR that might be candidates for a future PR.)

Definitely! :)

astrofrog · 2014-12-18T23:29:11Z

@cdeil - I've added a short paragraph about fits2bitmap

astrofrog · 2014-12-18T23:44:10Z

@cdeil - thanks for the detailed review! I think I managed to keep up. One unresolved issue is related to the script but I think that could be a more general bug and may be better addressed in a separate issue.

Does anyone else want to review this before tomorrow evening? :)

mwcraig · 2014-12-19T05:18:14Z

@astrofrog -- I'll try to give this a look Friday

cdeil · 2014-12-19T08:02:24Z

This looks great to me, thanks @astrofrog !
👍 to merge from my side.

saimn · 2014-12-19T11:17:45Z

-> astropy.visualization.scripts.fits2bitmap

Ahh, thanks! @cdeil, this explains the weird behavior with the script :)

astrofrog · 2014-12-19T22:23:56Z

@mhvk - just for info the transforms now use the inverse property.

eteq · 2014-12-19T22:39:47Z

These imports should be moved inside the function to minimize the chances of at-import ``ImportError`s

eteq · 2014-12-19T22:45:28Z

I think I'm happy with this aside from the one comment above.

I'm not a fan of the fact that this now introduces a module that triggers an error on import if you don't have an optional dep (mpl_normalize.py). Up to this point we've strictly kept to the policy that this isn't allowed, because anything that go through all the modules will now fail (i.e. sphinx if matplotlib is missing). But I realize in this case that we're subclassing, so you'd have to put the entire module in a try, which is pretty awkward. So we'll see if we encounter problems/complaints in the beta with this once people give it a spin...

…nal) even if matplotlib is not installed.

astrofrog · 2014-12-19T23:39:17Z

@eteq - ok, I addressed your comment in 67ddbc8 - now ImageNormalize is importable, though cannot be instantiated, if Matplotlib is not present.

astrofrog · 2014-12-19T23:41:23Z

This can be merged once Travis and AppVeyor pass

astrofrog · 2014-12-20T12:35:30Z

Checked over this again and it's good to go (and no objections above) so merging!

Merge imageutils functionality into astropy core

embray · 2014-12-22T17:33:47Z

👍 This was going to be my suggestion but looks like you're way ahead on that ;)

astrofrog force-pushed the astropy-image branch 3 times, most recently from 2b9ec49 to cbbec4d Compare December 14, 2014 14:34

astrofrog added this to the v1.0.0 milestone Dec 14, 2014

astrofrog added the 🔥 Critical label Dec 14, 2014

astrofrog self-assigned this Dec 14, 2014

astrofrog force-pushed the astropy-image branch 3 times, most recently from e7fcb3e to b422aaa Compare December 15, 2014 08:41

embray added Enhancement Affects-release labels Dec 15, 2014

embray reviewed Dec 15, 2014
View reviewed changes

cdeil reviewed Dec 18, 2014
View reviewed changes

Added a short paragraph about fits2bitmap

fc3244f

astrofrog added 2 commits December 18, 2014 23:36

Small fixes

7454e97

Removed whitespace

cc56d73

astrofrog mentioned this pull request Dec 19, 2014

Change sig to sigma in sigma_clip #3236

Closed

cdeil mentioned this pull request Dec 19, 2014

Documentation lacks top-level list of executable scripts #2185

Merged

adonath mentioned this pull request Dec 19, 2014

0.1 release astropy/photutils#180

Closed

saimn reviewed Dec 19, 2014
View reviewed changes

Fix fits2bitmap script

dc6a5cb

adonath mentioned this pull request Dec 19, 2014

Adapt psf.py to new array utils api in astropy.ndata.utils astropy/photutils#217

Closed

eteq reviewed Dec 19, 2014
View reviewed changes

astrofrog added 2 commits December 19, 2014 23:06

Move matplotlib imports inside function

d8f84c5

Ensure that ImageNormalize is importable (but not necessarily functio…

67ddbc8

…nal) even if matplotlib is not installed.

astrofrog added a commit that referenced this pull request Dec 20, 2014

Merge pull request #3201 from astrofrog/astropy-image

426f0d4

Merge imageutils functionality into astropy core

astrofrog merged commit 426f0d4 into astropy:master Dec 20, 2014

embray reviewed Dec 22, 2014
View reviewed changes

mhvk mentioned this pull request Jan 6, 2015

Updates to sigma_clipped_stats #3268

Merged

embray removed the Ready-for-final-review label Jan 13, 2015

cdeil mentioned this pull request Feb 5, 2015

downsample does not accept dtype=int astropy/imageutils#43

Open

larrybradley mentioned this pull request Feb 6, 2015

Add block_reduce and block_replicate functions #3453

Merged

astrofrog deleted the astropy-image branch July 5, 2016 15:44

Conversation

astrofrog commented Dec 14, 2014

Uh oh!

astrofrog commented Dec 15, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

embray commented Dec 15, 2014

Uh oh!

astrofrog commented Dec 15, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhvk commented Dec 15, 2014

Uh oh!

astrofrog commented Dec 15, 2014

Uh oh!

astrofrog commented Dec 15, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

astrofrog commented Dec 16, 2014

Uh oh!

cdeil commented Dec 16, 2014

Uh oh!

mhvk commented Dec 16, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

astrofrog commented Dec 18, 2014

Uh oh!

astrofrog commented Dec 18, 2014

Uh oh!

mwcraig commented Dec 19, 2014

Uh oh!

cdeil commented Dec 19, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

astrofrog commented Dec 19, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eteq commented Dec 19, 2014

Uh oh!

astrofrog commented Dec 19, 2014

Uh oh!

astrofrog commented Dec 19, 2014

Uh oh!

astrofrog commented Dec 20, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants