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

feat: add set_image_piecewise_function_points #755

Merged
merged 1 commit into from
Jun 24, 2024

Conversation

PaulHax
Copy link
Collaborator

@PaulHax PaulHax commented Jun 21, 2024

remove set_image_piecewise_function_gaussians

Depends on
Kitware/itk-vtk-viewer#751

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@PaulHax PaulHax marked this pull request as ready for review June 21, 2024 21:38
@PaulHax PaulHax requested a review from thewtex June 21, 2024 21:41
Copy link
Member

@thewtex thewtex left a comment

Choose a reason for hiding this comment

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

@PaulHax awesome, thank you!


Gaussian_Curve = Dict[str, float]
Gaussians = Dict[str, List[Gaussian_Curve]]
Points2d = List[Tuple[float, float]]
Copy link
Member

Choose a reason for hiding this comment

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

This could be

Sequence[Sequence[float, float]]

? Please check if variants work by passing them to the set_* method. If they do not work, then it may be worthwhile type casting with list() or tuple() to provide a more convenient interface for the caller.

In the documentation example there is:

[[.2, .1], [.8, .9]]

which is a List[List[float, float]].

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, yes. Updated type to Sequence[Sequence[float]]. Sequence can't do Sequence[float, float]. Passing tuple points or list points works. Passing 3D point bugs out TF editor =/

@@ -22,7 +22,7 @@ def init_params_dict(itk_viewer):
'gradient_opacity': itk_viewer.setImageGradientOpacity,
'gradient_opacity_scale': itk_viewer.setImageGradientOpacityScale,
'interpolation': itk_viewer.setImageInterpolationEnabled,
'gaussians': itk_viewer.setImagePiecewiseFunctionGaussians,
'piecewise_function_points': itk_viewer.setImagePiecewiseFunctionPoints,
Copy link
Member

Choose a reason for hiding this comment

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

How about tf_points or something else shorter here?

Copy link
Member

Choose a reason for hiding this comment

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

Or the more specific transfer_function_points?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changed to transfer_function_points

Copy link
Member

Choose a reason for hiding this comment

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

@PaulHax thank you! How about transfer_function for brevity? I do not think _points is adding enough information to warrant the length for interactive use. They are always points and this is implicit in the specification.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Better and better. Changed.

@thewtex thewtex dismissed their stale review June 22, 2024 20:04

resolved

@PaulHax PaulHax force-pushed the tf-points branch 2 times, most recently from 807300c to 883440d Compare June 23, 2024 17:41
remove set_image_piecewise_function_gaussians
Copy link
Member

@thewtex thewtex left a comment

Choose a reason for hiding this comment

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

💯

@thewtex thewtex merged commit 420355e into InsightSoftwareConsortium:main Jun 24, 2024
6 checks passed
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.

2 participants