-
Notifications
You must be signed in to change notification settings - Fork 15
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
rename parameters in add_images_weighted #376
Conversation
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #376 +/- ##
==========================================
- Coverage 48.44% 48.36% -0.09%
==========================================
Files 243 243
Lines 3965 3974 +9
Branches 1809 1818 +9
==========================================
+ Hits 1921 1922 +1
Misses 619 619
- Partials 1425 1433 +8 ☔ View full report in Codecov by Sentry. |
Sorry in advance for the pre-commit thing 😅 |
… into rename_parameters
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
@StRigaud This is ready for review. Feel free to merge if it's ok! |
Signed-off-by: Stephane Rigaud <[email protected]>
const Array::Pointer & src0, | ||
const Array::Pointer & src1, | ||
Array::Pointer dst, | ||
float factor0, | ||
float factor1) -> Array::Pointer; | ||
float factor1, | ||
float factor2) -> Array::Pointer; |
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.
I know it is to fit the prototype
but the input index being 0 and 1, and the factor index being 1 and 2 triggers my OCD.
Wouldn't we want to also use this effort to correct possible wrong doing in the API developpment during the prototype
( i know I am annoying 😉 )
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.
That's why it should also be input_image1 and input_image2, everywhere
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.
The documentation is also commonly "First image to process"
const Array::Pointer & src0, | ||
const Array::Pointer & src1, | ||
Array::Pointer dst, | ||
float factor0, | ||
float factor1) -> Array::Pointer | ||
float factor1, | ||
float factor2) -> Array::Pointer |
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.
same as first comment
This will require to update some of the tests in pyclesperanto because of the parameter name change but otherwise, should be ok. Just the factor indexing that annoys me a bit... the rest i can live with it 🙃 |
Here I'm renaming numeric parameters of some functions to improve backwards-compatibility with the prototype
closes #369
partially solves clEsperanto/pyclesperanto#260