Lacosmicx

[cmccully]

I have finished the optimized LA Cosmic implementation. It uses Cython for the main part of the algorithm and uses C for the individual routines. Everything is multithreaded using OpenMP (the Cython standard). For a 4K x 2K image, this implementation is ~35 times faster than cosmics.py written by Malte Tewes (which is already significantly faster than the original IRAF version).

Given the complexity of this code, should this be included in imageutils or should it be its own package?

Any other comments are welcome.

[cdeil]

@larrybradley and / or @crawfordsm Do you have time to review this?

[larrybradley]

I'll review this next week.

[astrofrog]

@cdeil @larrybradley - @cmccully raised a valid question as to whether this should be included in imageutils or should be its own package? Another option is to include it as part of ccdproc. I don't have a strong preference but just to be clear, the plan is to merge imageutils into astropy core within a couple of months, so this needs to be stable enough for merging API-wise.

[astrofrog]

This file should not be included here - it only needs to exist once, at the top-level of the package.

[eteq]

I think this has to be a dependency for photutils and ccdproc, right? If so, then it can't be part of either separately. And I think a separate package just for this doesn't make sense because it will be a pain to maintain in lockstep with photutils and ccdproc. So that makes me think it should go into imageutils. But I agree with @astrofrog it has to be at least reasonably API-stable if we do that.

[eteq]

Also, @cmccully, am I reading right that it does not parallelize on clang? I ask because I know a lot of the user base are on macs...

[astrofrog]

@cdeil - ok, but I think it would still be worth splitting out the files as I suggested in my last comment, since this would be low effort for now and would make things a lot clearer to tackle in future.

[yarikoptic]

@cdeil I am sorry I have missed your original question about a Debian machine. If you don't need anything "exotic" then you could get Debian running via docker, virtualbox (e.g. we provide neurodebian appliance) or vagrant. If you need access to some specific platform (e.g. sparc), email me ;)

[cdeil]

@yarikoptic - The question is exactly if python setup.py install and python setup.test for this pull request works with exotic machines or compilers. It's probably easiest to ask for someone to do this testing after it's merge into the Astropy core repo for an Astropy 1.0 release candidate ... unless it's easy / little work for you to run this on a few test machines.

[yarikoptic]

;-) well -- seems not even an exotic one necessary (tested on my laptop and sparc box) to fail install:

In file included from /usr/lib/python2.7/dist-packages/numpy/core/include/numpy/ufuncobject.h:327:0,
                 from astropy/convolution/boundary_none.c:240:
/usr/lib/python2.7/dist-packages/numpy/core/include/numpy/__ufunc_api.h:241:1: warning: ‘_import_umath’ defined but not used [-Wunused-function]
 _import_umath(void)
 ^
error: Setup script exited with error: [Errno 2] No such file or directory: 'imageutils/cython_version.py'
PYTHONPATH=$PWD/INSTALL/lib/python2.7/site-packages/ python setup.py install   110.85s user 3.21s system 84% cpu 2:14.92 total

@cdeil email me your public ssh key and desired login name, paste here the fingerprint of your ssh key and I will give you access to an exotic box so you could try yourself

[astrofrog]

By the way, if people want to go ahead and merge this we can do that and I'm happy to do the splitting out of the files as I mentioned above.

[larrybradley]

+1 for splitting out the general purpose functions, but that can happen in a separate PR. I also wanted to investigate some small differences (when I get time), but that too can be in a separate PR.

[astrofrog]

Ok, so it seems like we're just waiting for @eteq to confirm back what Pieter van Dokkum says, and if all is well on that front, we can merge and do further work on this before starting to do a PR for astropy.

[eteq]

I talked to him yesterday, and then sent him an e-mail to get it in writing (and answer a few questions about our license), and he hasn't replied yet. It's possible he's left for break...? Will try pinging him again.

[cmccully]

I am happy to refactor this code, but I won't have much time until next week after the holiday. Do you want to include that in this pull request or should we merge this and then refactor in a separate pull request?

[astrofrog]

@cmccully - we can do the refactoring in a separate pull request. Let's just wait for @eteq to give a go-ahead.

[eteq]

I suspect that will also have to wait until after the holiday, as I still haven't heard back.

[astrofrog]

@eteq - any updates on this?

[larrybradley]

Any updates on the licensing issue?

[coveralls]

Coverage increased (+0.07%) to 94.1% when pulling 90fa0bf on cmccully:lacosmicx into 3eafeb6 on astropy:master.

[cmccully]

The travis build now fails for two of the tests saying jinja2 is not installed. I think I am using the most up to date build of imageutils master. Anyone know how to fix this? Thanks.

[astrofrog]

@cmccully - in the file .travis.yml there is a line that says:

- if [[ $SETUP_CMD != egg_info ]]; then $CONDA_INSTALL numpy=$NUMPY_VERSION pytest pip Cython; fi

add jinja2 just after Cython, so:

- if [[ $SETUP_CMD != egg_info ]]; then $CONDA_INSTALL numpy=$NUMPY_VERSION pytest pip Cython jinja2; fi

basically jinja2 is now required to build the developer version of Astropy.

[astrofrog]

@eteq - any updates on the licensing issue?

[coveralls]

Coverage increased (+0.07%) to 94.1% when pulling d4caa56 on cmccully:lacosmicx into 3eafeb6 on astropy:master.

[mwcraig]

@eteq @astrofrog -- friendly bump...we are waiting for this to land somewhere so we remove functionality that duplicates this (but runs much more slowly) in ccdproc

[larrybradley]

lacosmic is now in the astroscrappy package.