Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove gammapy.detect.CWT #2535

Merged
merged 3 commits into from Nov 13, 2019
Merged

Remove gammapy.detect.CWT #2535

merged 3 commits into from Nov 13, 2019

Conversation

@cdeil
Copy link
Member

cdeil commented Nov 13, 2019

This PR removes gammapy.detect.CWT.

It will forever be available in Gammapy v0.14, here, if someone wants to use this, or improve the code and re-add it to Gammapy in the future:

IMO this functionality is in scope for Gammapy, but the quality of the code, tests and docs is too low to keep it and to offer it to users, and we should instead focus our efforts on Li & MA and TS code, tests and docs (see e.g. #2403).

I could enumerate the issues and work that should be done, but maybe this removal isn't controversial, so I'll not make the case for now. I think it would be a week of full-time work for an expert like @adonath or @registerrier to get this into shape, and to make sure the results are solid and the documentation is good. This was added 5+ years ago without tests, and then we had a GSoC student add tests and write the tutorial, but we didn't review the results at the time, and didn't understand the results we got for different parameters, I suspect it's not giving good results. This is also something that extremely few Gammapy users need, so it's not high priority for us, which explains why we haven't worked on this in the past years, even though it was clear that it needs work.

Of course there are many other things in Gammapy that need work, and in each case we have to make a decision to invest time to improve, or keep and improve later, or remove for now, with a comment that PRs in the future are welcome, with good quality implementation, tests and docs. In this case I think removing is best, if I were a potential contributor to Gammapy with wavelet skills, I'd probably be interested to add something, but not to clean up these 1000 lines of code and tests.

@registerrier @adonath - What do you think?

If you want to keep this, please make a proposal what to do with cwt.ipynb, as part of our upcoming documentation effort.

@cdeil cdeil added the cleanup label Nov 13, 2019
@cdeil cdeil added this to the 0.15 milestone Nov 13, 2019
@cdeil cdeil requested review from adonath and registerrier Nov 13, 2019
@cdeil cdeil added this to In progress in gammapy.detect via automation Nov 13, 2019
@cdeil

This comment has been minimized.

Copy link
Member Author

cdeil commented Nov 13, 2019

We should also discuss gammapy.detect.KernelBackgroundEstimator, gammapy.detect.ASmooth, the two ring background estimators, and the TS and Li&Ma code.

Those are in better shape than CWT, where it's 1000 lines of code, and it's most different from our overall API style (not using Map objects internally, two separate algorithm-specific CWD container classes, for maps and kernels). That's why I started with CWT. But IMO we should consider more removals, or improve and unify what we keep.

Copy link
Member

adonath left a comment

I'm fine to remove the CWT class now. It would probably much more work to refactor it, then to just rewrite it.

@adonath

This comment has been minimized.

Copy link
Member

adonath commented Nov 13, 2019

@cdeil I agree we have to talk about the other classes such as ASmooth, TSMapEstimator etc., but there I'm clearly against removing those (not sure you actually proposed it...). I think all of theses can be adapted with minimal effort to feature a uniform API (if this is not the case already), which probably means to adapt the API to use MapDataset and MapDatasetOnOff on input.

@cdeil

This comment has been minimized.

Copy link
Member Author

cdeil commented Nov 13, 2019

KernelBackgroundEstimator is similar to CWT in the sense that it's an iterative 2D image algorithm that produces a background model, exclusion mask and significance map. So we could remove it.

On the other hand, the one-step method of KernelBackgroundEstimator is a generalisation of ring background estimation, so ideally we would offer that general version and the ring methods without code duplication. Throwing away the general solution and only offering a ring kernel is weird. And even the iterative method is not uncommon - the iterated ring background estimation was the method used to make exclusion masks and background estimates for HGPS and other Galactic papers.

For ASmooth I have a similar concern as for CWT - it was never validated. A quick way to validate it could be to try to reproduce the 3FHL map - email the corresponding author if the asmooth map from 3FHL that's in the paper as a Figure isn't publicly available as a FITS file. But for ASmooth we can discuss in #751 and #752 - @adonath if you want to improve those this year, I suggest you leave a comment with action items, if you want to just do nothing this year, move back to v1.1 milestone.

@codecov

This comment has been minimized.

Copy link

codecov bot commented Nov 13, 2019

Codecov Report

Merging #2535 into master will decrease coverage by 0.07%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2535      +/-   ##
==========================================
- Coverage   91.48%   91.41%   -0.08%     
==========================================
  Files         145      144       -1     
  Lines       16550    16283     -267     
==========================================
- Hits        15141    14885     -256     
+ Misses       1409     1398      -11
Impacted Files Coverage Δ
gammapy/detect/__init__.py 100% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2826a93...9787c27. Read the comment docs.

Copy link
Contributor

registerrier left a comment

I think the CWT can be removed without any problem.
If it is to be reimplemented, it would be better to use existing wavelet transform implementations such as spicy.signal.cwt to significantly reduce the code and associated tests.
Having a detection tool as an input for modeling and fit can be useful. We have already TSMapEstimator, we will later figure out want kind of algorithm we want to provide. But this is really for post v1.0.
CWT was rather built to filter images.

@cdeil cdeil merged commit 3e6db71 into gammapy:master Nov 13, 2019
4 of 5 checks passed
4 of 5 checks passed
codecov/project 91.41% (-0.08%) compared to 2826a93
Details
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
Scrutinizer Analysis: No new issues – Tests: passed
Details
codecov/patch Coverage not affected when comparing 2826a93...9787c27
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
gammapy.detect automation moved this from In progress to Done Nov 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
3 participants
You can’t perform that action at this time.