Skip to content

Conversation

@tlambert03
Copy link
Contributor

This PR simplifies and fixes some bugs in the heavily used plugin_function decorator.
It moves to the more modern inspect.Signature API, and takes advantage of the Signature.bind method to accomplish in a single line what most of the previous implementation was trying to do.

Importantly, this is fixing a big bug in the current implementation, which is that position or keyword arguments were not working. as an explanation:

def f(x):
    print(x)

# in python a positional argument can also be called as a keyword argument:
f('hi')  # prints "hi"
f(x='hi')  # also prints "hi"
# this is super important because it lets you call a function in many different
# and dynamic ways:
keyword = 'x'
f(**{keyword: 'hi'})  # also prints "hi"

the current plugin_function breaks this functionality, meaning that functions must be called in a very specific way, or they will break:

from pyclesperanto_prototype import absolute
import numpy as np
r = np.random.rand(10,10) - 0.5

absolute(r)  # works fine
absolute(source=r)  # raises TypeError
pyclesperanto_prototype/pyclesperanto_prototype/_tier0/_execute.py in execute(anchor, opencl_kernel_filename, kernel_name, global_size, parameters, prog, constants, image_size_independent_kernel_compilation)
    211             arguments.append(np.array([value], np.float32))
    212         else:
--> 213             raise TypeError(
    214                 f"other types than float and int aren`t supported yet for parameters {key} : {value} . \n"
    215                 f"function {kernel_name}"

TypeError: other types than float and int aren`t supported yet for parameters src : [[ 0.42569358  0.41798918  0.487596    0.21005139 -0.19382348 -0.40420797

this PR fixes that...

however, I'm still having a bit of an issue with tests, because there was one more thing I'm struggling to figure out here. The output_creator function may be one of many of the functions in _create.py, however each of those has a very different signature (some take *args, some do not) ... so there too, extra logic is required in plugin_function to call the output_creator in just the right way depending on the context. Can you help me understand the intent there? Should output_creator usually just receive the positional arguments provided to the function? something different?

@haesleinhuepf
Copy link
Member

Awesome @tlambert03 , the new code is so short! ❤️

Closes #94

I'll send a PR to your repo for fixing the failing tests...

@codecov-io
Copy link

Codecov Report

Merging #95 (2333f21) into master (d9abb4f) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #95      +/-   ##
==========================================
- Coverage   91.33%   91.31%   -0.02%     
==========================================
  Files         474      474              
  Lines        7478     7462      -16     
==========================================
- Hits         6830     6814      -16     
  Misses        648      648              
Impacted Files Coverage Δ
pyclesperanto_prototype/_tier0/_plugin_function.py 100.00% <100.00%> (ø)
...prototype/_tier2/_maximum_of_touching_neighbors.py 90.90% <100.00%> (ø)
...to_prototype/_tier2/_mean_of_touching_neighbors.py 90.90% <100.00%> (ø)
..._prototype/_tier2/_median_of_touching_neighbors.py 45.45% <100.00%> (ø)
...prototype/_tier2/_minimum_of_touching_neighbors.py 90.90% <100.00%> (ø)
...to_prototype/_tier2/_mode_of_touching_neighbors.py 90.90% <100.00%> (ø)
...tier2/_standard_deviation_of_touching_neighbors.py 90.90% <100.00%> (ø)

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 d9abb4f...2333f21. Read the comment docs.

@tlambert03
Copy link
Contributor Author

tlambert03 commented Jan 12, 2021

thanks! This is mergeable now... I still think there's some room for improvement on the output_creator functions. Specifically I don't like that we have to call it like this output_creator(*bound.args[:len(args)]) (because len(args) there is dependent on how the outer function was called... and that's a little wonky). I think the fix would require modifying the create functions themselves to have a unified calling signature, but I'm not sure how widely those functions are used elsewhere, so not touching that here.

@haesleinhuepf
Copy link
Member

haesleinhuepf commented Jan 12, 2021

I don't like that we have to call it like this output_creator(*bound.args[:len(args)]) (because len(args) there is dependent on how the outer function was called...

Yes! I also don't like that either. I struggled hard to make it work earlier and would love to come up with a better solution. You should see the solution on Java side.... It's sooo much worse ;-)

I will continue thinking about it.

Thanks for this PR!

@haesleinhuepf haesleinhuepf merged commit b9d6c64 into clEsperanto:master Jan 12, 2021
@tlambert03 tlambert03 deleted the fix-plugin-func branch January 13, 2021 00:09
@codecov-commenter
Copy link

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.31%. Comparing base (d9abb4f) to head (2333f21).
Report is 1047 commits behind head on master.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #95      +/-   ##
==========================================
- Coverage   91.33%   91.31%   -0.02%     
==========================================
  Files         474      474              
  Lines        7478     7462      -16     
==========================================
- Hits         6830     6814      -16     
  Misses        648      648              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants