Skip to content
This repository was archived by the owner on Dec 15, 2020. It is now read-only.

Add photutils.utils functions - #11

Merged
cdeil merged 7 commits into
astropy:masterfrom
larrybradley:photutils
Aug 4, 2014
Merged

cdeil merged 7 commits into
astropy:masterfrom
larrybradley:photutils

Conversation

@larrybradley

Copy link
Copy Markdown
Member

I added everything that was in photutils.utils (re: #9)

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

@cdeil

cdeil commented Jul 30, 2014

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Member Author

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

@larrybradley

Copy link
Copy Markdown
Member Author

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

Can I get privileges for Travis CI?

Comment thread imageutils/__init__.py Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The travis-ci setup-egg info builds are failing because of this:
https://travis-ci.org/astropy/imageutils/jobs/31270906

You have to put all imports into the if not _ASTROPY_SETUP_: block.

@cdeil

cdeil commented Jul 30, 2014

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Member Author

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

@larrybradley

Copy link
Copy Markdown
Member Author

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

Copy link
Copy Markdown
Member Author

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

@cdeil

cdeil commented Aug 4, 2014

Copy link
Copy Markdown
Member

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

cdeil commented Aug 4, 2014

Copy link
Copy Markdown
Member

Thanks!

cdeil added a commit that referenced this pull request Aug 4, 2014
Add photutils.utils functions
@cdeil
cdeil merged commit 5d95d98 into astropy:master Aug 4, 2014
@larrybradley

Copy link
Copy Markdown
Member Author

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

cdeil commented Aug 4, 2014

Copy link
Copy Markdown
Member

@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

cdeil commented Aug 4, 2014

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Member Author

@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

Copy link
Copy Markdown
Member

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

cdeil commented Aug 4, 2014

Copy link
Copy Markdown
Member

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

cdeil commented Aug 4, 2014

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Member Author

@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

cdeil commented Aug 4, 2014

Copy link
Copy Markdown
Member

@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.)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants