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

Improve continuous wavelet transform #661

Merged
merged 1 commit into from Aug 23, 2016
Merged

Conversation

OlgaVorokh
Copy link
Member

@OlgaVorokh OlgaVorokh commented Aug 3, 2016

This PR improves CWT, which was added in #25 and recently improved directly in Gammapy master.

On this PR the follow work was done:

  • quality of CWT code was improved
  • all the data images and information about using kernels was been separated from CWT class into CWTData and CWTKernels classes
  • the tests were added for all the created classes
  • docs was been modernized
  • ipython tutorial was written for CWT algorithm (see here)

R. Terrier, L. Demanet, I.A. Grenier, and J.P. Antoine. Wavelet
analysis of EGRET data. Universite Paris VII & Sap CEA Saclay.
Institut de physique theorique et mathematique, Universite
catholique de Louvain, Louvain-la-Neuve
Copy link
Contributor

Choose a reason for hiding this comment

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

The most important thing to add is the link to http://adsabs.harvard.edu/abs/2001ICRC....7.2923T so that others can quickly find what publication exactly you are referencing.
The description can be shortened to only contain the key information like this:

R. Terrier et al (2001) "Wavelet analysis of EGRET data"
See http://adsabs.harvard.edu/abs/2001ICRC....7.2923T

Copy link
Member Author

@OlgaVorokh OlgaVorokh Aug 4, 2016

Choose a reason for hiding this comment

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

OK, I'll fixed it now.

@OlgaVorokh
Copy link
Member Author

OlgaVorokh commented Aug 4, 2016

@cdeil @adonath I prefer to implement the follow style of using CWT:

cwt = CWT(... set parameters... )
data = .. some data, maybe CWTData ...
cwt.analyze(data)

So we firstly setup the parameters and than start to analyze data and after computing we can use created CWT for analyzing other data.

What are you thinking about it?

@cdeil
Copy link
Contributor

cdeil commented Aug 4, 2016

No strong opinion on this, maybe small +1 to what you propose, to use the sklearn style of passing data only later to run or whatever it's called and not already to __init__.

@cdeil
Copy link
Contributor

cdeil commented Aug 4, 2016

I think my preference would be to split out the scales and kernels into a separate class.
Mostly to have it nicely grouped, not necessarily for code re-use from other applications, although that might be possible later.

class CWTKernels:
    def __init__(self, scales):
        scales = np.array(scales)
        # compute kernels here, like previously done in `CWT.__init__`

    @classmethod
    def from_scale_grid(cls, n, min, step):
        scales = [min * step ** _ for _ in range(n)]
        return cls(scales)

    def __str__():
        # print useful info about the grid of scales, can be added later

We will be able to re-use that class for other multi-scale algorithms, so I think it's good to split it out now. To use it for CWT:

from gammapy.detect import CWTKernels, CWT

kernels = CWTKernels.from_scale_grid(... parameters ...)
print(scales) # for debugging
cwt = CWT(scales)
cwt.run(data)

OK to split out the scale / kernel handling this way, or do you prefer to keep the scales / kernel handling in CWT.__init__?

self.support = None

# Strange result after each iteration of CWT...
self.mysterious_result = None

@property
def max_scale_image(self):
Copy link
Member Author

@OlgaVorokh OlgaVorokh Aug 5, 2016

Choose a reason for hiding this comment

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

@cdeil I moved this method here, but as you can see it will be not work, because we don't have some information about the scales in that class. Maybe it will be better to create kernel class, but I don't know

# Strange result after each iteration of CWT...
self.mysterious_result = np.zeros(shape_2d)
@property
def max_scale_image(self):
Copy link
Member Author

Choose a reason for hiding this comment

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

If I understand correct, this method should look like this. As I understand, maximal image should be created on the same way as we create data.transform_2d in the CWT._inverse_transform.

@OlgaVorokh OlgaVorokh force-pushed the improve_cwt branch 2 times, most recently from dda5643 to 4353999 Compare August 20, 2016 15:37
@cdeil
Copy link
Contributor

cdeil commented Aug 23, 2016

Merging this no.

@OlgaVorokh - Thank you for this big contribution and your patience with review / merging on this PR!

@cdeil cdeil merged commit e55c751 into gammapy:master Aug 23, 2016
@cdeil cdeil changed the title Improve detect.CWT Improve continuous wavelet transform Aug 23, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants