WOW - proposed solution to issue 187

[frederic-auchere]

Proposed solution to issue 187

Addition of a hook to the wow function of the pip-installable watroo package. As I did not want to force watroo to be a dependency of sunkit-image I've made the import optional in enhance.py. I suppose therefore that watroo should be an option in the setup process. I don't know how to do that however, and I did not want to mess with the setup files. I'll need some help with that :)

[hayesla]

Hi @frederic-auchere thanks for this PR, and it would be great to have this available through sunkit-image! :)

I think as you say it could of course be an optional dependency, but to do this we would need to have it available on pypi so that it can be installed via pip. So is this something you could do? Then we could set it up in the config that watoo is an optional dependency on install. If you need any assistance with this, let us know!

Then once its on pypi ), a test and an example of using WOW would also be great to add to this PR!

[hayesla]

another option is to put the whole thing in sunkit-image if this was something you'd be interested in?

[frederic-auchere]

I think as you say it could of course be an optional dependency, but to do this we would need to have it available on pypi so that it can be installed via pip. So is this something you could do? Then we could set it up in the config that watoo is an optional dependency on install. If you need any assistance with this, let us know!

Then once its on pypi ), a test and an example of using WOW would also be great to add to this PR!

The watroo package is already in pypi :) I can easily add an example. Do you need that as an example command line in the docstring? I could also put together a small example in the sunpy gallery. I'll need guidance on how to do that ;)

[frederic-auchere]

another option is to put the whole thing in sunkit-image if this was something you'd be interested in?

Well, there is a bunch of wavelet stuff in the watroo package that WOW uses but that is more generic. So watroo would still need to be a dependency.

[hayesla]

Hi @frederic-auchere ! my apologies for the delay in getting back to you about this!

Great that it's on pypi already :) we've discussed it further, and think it makes sense to live in sunkit-instr even as a wrapper around your package.

What this PR now needs is:

1. Some changes to the function signature call

• for this function to be within sunpy, we could need all the arguments within the function call, rather than just *arg, **kwargs

2. Some tests that makes sure that its doing what we think its doing

   • We would need to add at least one test, that would live in here: https://github.com/sunpy/sunkit-image/blob/main/sunkit_image/tests/test_enhance.py. You can see how the other tests are written.

3. An example gallery entry

   • Adding an example .py file to here: https://github.com/sunpy/sunkit-image/tree/main/examples this can just be some simple python code, that is written it such a way that it can be generated nicely on the docs Here is an example template.

We can also add watroo as an optional dependency (here).

If you would like, I could add some commits directly to this PR to address some of these, or if you would like more details about anything here let us know!

Thanks!

[frederic-auchere]

Hi @hayesla,

Thank you for the time to review this :)

1. Concerning the signature, I've done it as *args, **kwargs so that changes in the signature in the watroo package would not have to be replicated systematically. Now I can of course make the change, but I would see that as a risk. Let me know.

2. I've started to include tests in watroo, I've also added a very basic one in sunkit. I failed to run pytest howver. Not because of my test, but running pytest I get this error :

ERROR: usage: pytest [options] [file_or_dir] [file_or_dir] [...]
pytest: error: unrecognized arguments: --doctest-rst

and trying python setup.py test (which I beleive is deprecated) I get lots of warnings and 0 tests ran. I'll need help with this ;)

3. I've added an example also.

4. There I may need your help :) There is code in already in enhance.py to handle watroo as on optional import, but I don't know what should be done in the toml file (if anything) !

[nabobalis]

1. Concerning the signature, I've done it as *args, **kwargs so that changes in the signature in the watroo package would not have to be replicated systematically. Now I can of course make the change, but I would see that as a risk. Let me know.

My personal thought is that I want to make sure that as we are passing any arg or keywords to WOW, we should make sure that its correct and to do that we have two choices:

1. We just copy the WOW function signature here
2. We inspect the WOW function and raise a warning/error if the input args/keywords do not match the signature.

2. I've started to include tests in watroo, I've also added a very basic one in sunkit. I failed to run pytest howver. Not because of my test, but running pytest I get this error :

ERROR: usage: pytest [options] [file_or_dir] [file_or_dir] [...]
pytest: error: unrecognized arguments: --doctest-rst

So the tests require a series of external plugins, those are installed (I hope) when you pip install -e ".[tests]"

Your other choice is pip install tox and run the test suite that way by doing tox -e py311 (or whatever your python version is).

and trying python setup.py test (which I beleive is deprecated) I get lots of warnings and 0 tests ran. I'll need help with this ;)

Yeah, direct setup.py use is going away and that file will also go away in the future.

3. I've added an example also.
4. There I may need your help :) There is code in already in enhance.py to handle watroo as on optional import, but I don't know what should be done in the toml file (if anything) !

Here (https://github.com/sunpy/sunkit-image/blob/main/pyproject.toml#L50C1-L51C23), you will want to create a new group, that installs wow. The question is what to call this group, maybe enhance since it is functions within that file?

[frederic-auchere]

2. We inspect the WOW function and raise a warning/error if the input args/keywords do not match the signature.

Can you point me to an example on how to do that?

So the tests require a series of external plugins, those are installed (I hope) when you pip install -e ".[tests]"

pytest now runs, but I get the following

=============================================================================================================== ERRORS ================================================================================================================ 
______________________________________________________________________________________________ ERROR collecting sunkit_image/enhance.py _______________________________________________________________________________________________ 
sunkit_image\enhance.py:163: in <module>
    @accept_array_or_map(arg_name="data")
sunkit_image\utils\decorators.py:34: in decorate
    raise RuntimeError(msg)
E   RuntimeError: Could not find 'data' in function signature
sunkit_image\tests\test_enhance.py:7: in <module>
    import sunkit_image.enhance as enhance
sunkit_image\enhance.py:163: in <module>
    @accept_array_or_map(arg_name="data")
sunkit_image\utils\decorators.py:34: in decorate
    raise RuntimeError(msg)
E   RuntimeError: Could not find 'data' in function signature
======================================================================================================= short test summary info ======================================================================================================= 
ERROR sunkit_image/enhance.py - RuntimeError: Could not find 'data' in function signature
ERROR sunkit_image/tests/test_enhance.py - RuntimeError: Could not find 'data' in function signature
ERROR sunkit_image/tests/test_enhance.py - RuntimeError: Could not find 'data' in function signature
!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! Interrupted: 3 errors during collection !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! 
========================================================================================================== 3 errors in 1.08s ========================================================================================================== ```


Any idea?

[nabobalis]

2. We inspect the WOW function and raise a warning/error if the input args/keywords do not match the signature.

Can you point me to an example on how to do that?

Something like: https://www.geeksforgeeks.org/how-to-get-list-of-parameters-name-from-a-function-in-python/

But it was more of a general comment/question, how best should we handle this?

My personal view is that if the function in WOW changes, I want to try and make sure that users of WOW via sunkit-image are alerted if a arg or keyword is unused or changed.

I don't think it would be trivial to solve like this, which is why I think that copying the args is easier/quicker but might not be the best solution.

So the tests require a series of external plugins, those are installed (I hope) when you pip install -e ".[tests]"

pytest now runs, but I get the following

=============================================================================================================== ERRORS ================================================================================================================ 
______________________________________________________________________________________________ ERROR collecting sunkit_image/enhance.py _______________________________________________________________________________________________ 
sunkit_image\enhance.py:163: in <module>
    @accept_array_or_map(arg_name="data")
sunkit_image\utils\decorators.py:34: in decorate
    raise RuntimeError(msg)
E   RuntimeError: Could not find 'data' in function signature
sunkit_image\tests\test_enhance.py:7: in <module>
    import sunkit_image.enhance as enhance
sunkit_image\enhance.py:163: in <module>
    @accept_array_or_map(arg_name="data")
sunkit_image\utils\decorators.py:34: in decorate
    raise RuntimeError(msg)
E   RuntimeError: Could not find 'data' in function signature
======================================================================================================= short test summary info ======================================================================================================= 
ERROR sunkit_image/enhance.py - RuntimeError: Could not find 'data' in function signature
ERROR sunkit_image/tests/test_enhance.py - RuntimeError: Could not find 'data' in function signature
ERROR sunkit_image/tests/test_enhance.py - RuntimeError: Could not find 'data' in function signature
!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! Interrupted: 3 errors during collection !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! 
========================================================================================================== 3 errors in 1.08s ========================================================================================================== ```


Any idea?

Ah I forgot, with *args, **kwargs I am not sure if that decorator will work. What is the name of the arg into that function for WOW?

[frederic-auchere]

I don't think it would be trivial to solve like this, which is why I think that copying the args is easier/quicker but might not be the best solution.

Let's copy the signature if you think it's best, then. I'll do that.

Ah I forgot, with *args, **kwargs I am not sure if that decorator will work. What is the name of the arg into that function for WOW?

def wow(data,
        scaling_function=B3spline,
        n_scales=None,
        weights=[],
        whitening=True,
        denoise_coefficients=[],
        noise=None,
        bilateral=None,
        bilateral_scaling=False,
        soft_threshold=True,
        preserve_variance=False,
        gamma=3.2,
        gamma_min=None,
        gamma_max=None,
        h=0):

[nabobalis]

I don't think it would be trivial to solve like this, which is why I think that copying the args is easier/quicker but might not be the best solution.

Let's copy the signature if you think it's best, then. I'll do that.

Ah I forgot, with *args, **kwargs I am not sure if that decorator will work. What is the name of the arg into that function for WOW?

def wow(data,
        scaling_function=B3spline,
        n_scales=None,
        weights=[],
        whitening=True,
        denoise_coefficients=[],
        noise=None,
        bilateral=None,
        bilateral_scaling=False,
        soft_threshold=True,
        preserve_variance=False,
        gamma=3.2,
        gamma_min=None,
        gamma_max=None,
        h=0):

Yeah I would copy them wholescale in this function for now.

Edit: Oh, B3spline might complicate that. We might need to set that to None and if its none, then import that from WOW and use it?

[frederic-auchere]

I don't think it would be trivial to solve like this, which is why I think that copying the args is easier/quicker but might not be the best solution.

Let's copy the signature if you think it's best, then. I'll do that.

Ah I forgot, with *args, **kwargs I am not sure if that decorator will work. What is the name of the arg into that function for WOW?

def wow(data,
        scaling_function=B3spline,
        n_scales=None,
        weights=[],
        whitening=True,
        denoise_coefficients=[],
        noise=None,
        bilateral=None,
        bilateral_scaling=False,
        soft_threshold=True,
        preserve_variance=False,
        gamma=3.2,
        gamma_min=None,
        gamma_max=None,
        h=0):

Yeah I would copy them wholescale in this function for now.

Edit: Oh, B3spline might complicate that. We might need to set that to None and if its none, then import that from WOW and use it?

OK, did that !

[nabobalis]

Thank you and sorry for my late response.

Would it be ok, if I debug the doc problems and push a fix?

[frederic-auchere]

Thank you and sorry for my late response.

Would it be ok, if I debug the doc problems and push a fix?

I have no idea what the doc problems is :) So sure, please go ahead !

[nabobalis]

I ended up finding a bunch of problems that were unrelated to your changes that I couldn't resist not fixing. So this PR kind of ballooned out of control.

[nabobalis]

Thank you for putting up with us @frederic-auchere