search

astropy / imageutils

Image processing utilities for Astropy (No longer maintained)
https://imageutils.readthedocs.org/
9 stars 17 forks source link

Add photutils.utils functions #11

Closed larrybradley closed 9 years ago

larrybradley commented 9 years ago

I added everything that was in photutils.utils (re: https://github.com/astropy/imageutils/issues/9)

Should fix_prf_nan stay in photutils.utils? It seems more specific to photutils.

cdeil commented 9 years ago

I think fix_prf_nan is not really prf-specific ... how about renaming it to nan_to_mirrored_num (in analogy to numpy.nan_to_num?

Actually there's a few things that could be improved about those functions ... @larrybradley do you want any code review in this PR? Maybe best to only do the move here?

larrybradley commented 9 years ago

@cdeil OK, I'll rename it and leave it here. Yeah, let's complete the move from photutils before any code review.

larrybradley commented 9 years ago

Some of the Travis CI tests are failing to import numpy. I need to investigate.

Can I get privileges for Travis CI?

cdeil commented 9 years ago

Can I get privileges for Travis CI?

I think travis-ci asks Github if you have push priviliges and if yes you can restart builds on travis-ci. So you should see the restart icon on the upper right as well as the merge pull request button here. Is this not the case?

larrybradley commented 9 years ago

@cdeil From my travis-ci account panel, I had to resync repositories from github. I can restart now.

larrybradley commented 9 years ago

I replaced fix_prf_nan with a new more general and flexible function called mask_to_mirrored_num. Anyone care to review that function before I merge this?

fix_prf_nan was actually (sort of) specific to PRFs. It assumes that the central pixel is the center of the array (it uses negative indices). Also, if the mirrored pixel was also masked, then it tried to mirror in x or y only, which is not applicable to a non-round object.

larrybradley commented 9 years ago

I should add that mask_to_mirrored_num is mostly taken directly from the soon-to-be-removed mirroring in photutils.

cdeil commented 9 years ago

I think there's a few things we should talk about, like function names and whether it's possible to group the scale_* functions somehow to have a smaller API. But as @larrybradley said above, let's keep this a simple move from photutils -> imageutils and everyone's free to open issues or PRs with improvements in the future.

cdeil commented 9 years ago

Thanks!

larrybradley commented 9 years ago

I'm thinking about combining the scale_* functions (to scale_image) and adding a scaling parameter (e.g. scaling='linear', scaling='log', etc. for the various types of scaling). Does that sound like a good idea?

I also have a command-line script that uses the scale_* functions to convert FITS files to scaled bitmap files that I can add if desired.

cdeil commented 9 years ago

@larrybradley Yes, I'd prefer fewer functions and a string option to keep the API small.

Did you consider adding this functionality to skimage.exposure? I'd prefer that.

Can you open a new issue to discuss this and maybe also cc Tom Robitaille and Erik Jeschke ... I haven't looked at their code, but they probably have image scaling functions for APLPy, wcsaxes or ginga and we should at least ask if they'd be willing to use yours to remove code duplication.

cdeil commented 9 years ago

About the command line script ... I have no use for it, but there's probably users that would welcome it ... maybe make a separate issue asking others if they think we should add it?

larrybradley commented 9 years ago

@cdeil I thought the plan is to move imageutils to astropy.image soon. Are we going to add scikit-image as a dependency of astropy?

astrofrog commented 9 years ago

I don't think there's any reason why scikit-image couldn't be an optional dependency of astropy (we already have optional dependencies on h5py, scipy, beautifulsoup, matplotlib, and maybe others)

cdeil commented 9 years ago

Are we going to add scikit-image as a dependency of astropy?

I'm not sure everyone agrees with that, but yes, I'm strongly in favour to use scikit-image as an optional dependency for astropy.image (or astropy.imageutils ... the name hasn't been really discussed or deded), just like we use scipy.ndimage already. Otherwise we'll end up duplicating a lot of the image utils they've done over the years (and will do based on feature requests and contributions by us).

cdeil commented 9 years ago

The hope is that the Python distribution situation continues to improve (e.g. pip and conda are getting better every year), so very few people will have a problem installing scikit-image ... actually that might already be the case.

larrybradley commented 9 years ago

@cdeil I see that skimage.exposure is using separate functions (e.g. adjust_gamma and adjust_log). They also scale the entire image dynamic range without allowing min/max cut levels.

cdeil commented 9 years ago

@larrybradley Any bad API or missing functionality in skimage can be changed with a pull request. But I realise it's (sometimes probably a lot) of extra work and discussion, so if you prefer to put your things in imageutils directly to get things done more quickly, OK. (I wanted to contribute some things to skimage for a long time, but never found the time to do so up to now.)