-
-
Notifications
You must be signed in to change notification settings - Fork 44
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
WOW - proposed solution to issue 187 #188
Conversation
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! |
another option is to put the whole thing in sunkit-image if this was something you'd be interested in? |
The watroo package is already in pypi :) |
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. |
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:
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! |
Hi @hayesla, Thank you for the time to review this :)
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 ;)
|
sunkit_image/tests/test_enhance.py
Outdated
_ = enhance.wow(aia_171.data) | ||
|
||
|
||
@pytest.mark.xfail |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is failing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't know I can't manage to run pytest on sunkit-image. I likely not running the right configuration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thats not a problem, we can use the CI on Github to check! I do it all the time.
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:
So the tests require a series of external plugins, those are installed (I hope) when you 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).
Yeah, direct setup.py use is going away and that file will also go away in the future.
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? |
Can you point me to an example on how to do that?
pytest now runs, but I get the following
|
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.
Ah I forgot, with |
Let's copy the signature if you think it's best, then. I'll do that.
|
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? |
Let's copy the signature if you think it's best, then. I'll do that.
OK, did that ! |
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 :) |
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. |
Thank you for putting up with us @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 forcewatroo
to be a dependency of sunkit-image I've made the import optional in enhance.py. I suppose therefore thatwatroo
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 :)Fixes #187