Unnecessary VTK API change

Development
1

There have been some breaking changes in vtkThreshold API that did not bring any value to VTK users. This is annoying, because discovering and fixing issues costs us time and there is zero benefit. It makes things even worse that the API change has just taken away convenience functions, which made vtkThreshold filter simpler and less error-prone to use.

Please consider the followings:

  • Restore vtkThreshold’s ThresholdByLower, ThresholdByUpper, ThresholdBetween methods that were removed in this commit (or give a convincing explanation why they must go).
  • Avoid upsetting VTK users by changing the API just “to make things nicer”. If something is not nice enough then take a note of it as a comment in the code or in the issue tracker but leave the API unchanged, until you have to change it because it is required to fix or improve something around there. New classes or features may be excepted, because some API churn is understandable there, but methods that have been around for several years should stay the same unless there is very strong reason for change.
1 Like
2

Hi @lassoan

I’m the one who designed the new vtkThreshold API, so thanks for using it :slight_smile:

vtkThreshold API, while functionnal, was definitely non standard, as the Treshold* method actually triggered the computation instead of configuring the filter before computation is handled by RequestData, making the filter unusable in a generic context (like ParaView), so the redesign decision was made.

The commit you link is just the usual “remove deprecated methods after one version of being deprecated”. The actual change I mention is this one. As you can see, the methods you mention were cleanly deprecated with a nice deprecation message explaining how to fix the issue.

There is also a nice documentation about the change that can be found in the release notes:
https://gitlab.kitware.com/vtk/vtk/-/blob/master/Documentation/release/9.1.md

And the more complete version:
https://gitlab.kitware.com/vtk/vtk/-/merge_requests/8262/diffs#diff-content-21ff1f33806647613f37157baf19a326410c3145

So to adress your specific comments:

This is annoying, because discovering and fixing issues costs us time and there is zero benefit.

Update VTK minor version by minor version with VTK_LEGACY_REMOVE=OFF and VTK_LEGACY_SILENT=OFF (the default) and read compilation warnings. This will bring the cost down to nothing. Also you may want to read release notes.

Restore vtkThreshold’s ThresholdByLower, ThresholdByUpper, ThresholdBetween methods

This will not happen I’m afraid.

Avoid upsetting VTK users by changing the API just “to make things nicer”. If something is not nice enough then take a note of it as a comment in the code or in the issue tracker but leave the API unchanged, until you have to change it because it is required to fix or improve something around there.

Well it was required to expose these features in ParaView. API changes were thorougly documented and old method deprecated. The only things that we could have done better would be to open a discourse thread, but it for a change regarding a single filter it felt unecessary at the time.

I hope that answers your questions.

FYI @Tiffany_Chhim

1 Like
3

Earlier post on this general topic: About API breaking changes

4

I cannot complain about how the breaking API change was made. With all the deprecation macros, etc. it was managed well.

The problem is that the change was made at all, as it requires efforts from VTK users to update their code, and the result is worse than before. The syntax was changed from:

threshold.ThresholdBetween(labelValue, labelValue)

to

threshold.SetLowerThreshold(labelValue)
threshold.SetUpperThreshold(labelValue)
threshold.SetThresholdFunction(vtk.vtkThreshold.THRESHOLD_BETWEEN)

vtkThreshold API, while functionnal, was definitely non standard, as the Treshold* method actually triggered the computation instead of configuring the filter before computation is handled by RequestData

It was just a convenience function. There was no direct triggering of any computation.

The same ThresholdByUpper(), ThresholdByLower(), ThresholdBetween() methods are still used in vtkImageThreshold.

Another worrying thing that I saw was that no convenience methods were added along with the introduction of SetThresholdFunction(int function). According to VTK style, these methods should have been added:

  • SetThresholdFunctionToBetween()
  • SetThresholdFunctionToUpper()
  • SetThresholdFunctionToLower()

These convenience methods are important, because they are much more convenient to use (especially with auto-complete) and have a much simpler syntax. This:

threshold.SetThresholdFunctionToBetween()

is much easier to write and read than what is required now:

threshold.SetThresholdFunction(vtk.vtkThreshold.THRESHOLD_BETWEEN)

If it was just a mistake then it is OK, it can be easily fixed. But if it was a conscious decision to skip them (to reduce VTK developer’s workload at the expense of user convenience) then that would need to be discussed, too.

5

It was just a convenience function. There was no direct triggering of any computation.

Correct I mispoke, however the problem was that with one method you control two parameters, which is definitely non-standard for a vtk filter. However in that context they could have been conserved but we made the conscious choice to remove them choosing to have a better API in the expense of changing it.

The same ThresholdByUpper(), ThresholdByLower(), ThresholdBetween() methods are still used in vtkImageThreshold.

It should be changed too.

. According to VTK style, these methods should have been added:

SetThresholdFunctionToBetween()
SetThresholdFunctionToUpper()
SetThresholdFunctionToLower()

Definitely ! That is a needed adition to make the API better. I’d gladly review a MR adding that in.

6

Hi @mwestphal - I understand your concern about having a cleaner API, but please take a moment to consider Bill Lorensen’s comments about backwards compatibility in the thread that @estan kindly linked above

7

HI @pieper

I’ve already responded to Bill on that thread. I still hold the same opinion.

Since that discussion, @ben.boeckel suggestion on deprecation mechanism have been implemented and used by VTK developers with great success. Not sure what more can be done.

VTK do not pledge to never change it’s API, it pledges to change its API using deprecation mechanism that always leave one minor version to adapt to the changes.

8

Let’s take it as a given that these are not easy decisions and the ones putting in the work deserve our appreciation for keeping the code maintained. Let’s also try to learn from each decision and try to make things easier for all of us going forward. I empathize with @lassoan and I can easily imagine myself with some broken code someday that required tracing back to find the replacement for ThresholdBetween and wondering why it was removed. I’d like to see VTK avoid that situation as much as possible and keep the API change bar set pretty high, deprecation warnings or not.

1 Like
9

Without getting into the pros and cons of changing the API, couldn’t the convenience method, vtkThreshold::ThresholdBetween, just be refactored into a static function in the downstream code ::ThresholdBetween(vtkThreshold* obj, double lower, double upper) and then updated with a search/replace?

1 Like
10

Redundant API for user convenience is standard in VTK filters.

Here are a few examples to illustrate.

vtkPlaneSource
vtkSetMacro(XResolution, int);
vtkSetMacro(YResolution, int);
void SetResolution(const int xR, const int yR);

vtkImageExtractComponents
void SetComponents(int c1);
void SetComponents(int c1, int c2);
void SetComponents(int c1, int c2, int c3);
vtkGetVector3Macro(Components, int);

vtkImageMask
void SetMaskedOutputValue(int num, double* v);
void SetMaskedOutputValue(double v) { this->SetMaskedOutputValue(1, &v); }
void SetMaskedOutputValue(double v1, double v2)
void SetMaskedOutputValue(double v1, double v2, double v3)

vtkImageGaussianSmooth
vtkSetVector3Macro(StandardDeviations, double);
void SetStandardDeviation(double std) { this->SetStandardDeviations(std, std, std); }
void SetStandardDeviations(double a, double b) { this->SetStandardDeviations(a, b, 0.0); }
vtkGetVector3Macro(StandardDeviations, double);

vtkImageSeedConnectivity
void AddSeed(int num, int* index);
void AddSeed(int i0, int i1, int i2);
void AddSeed(int i0, int i1);

Although these method variants are not strictly necessary, they must not be removed because:

  1. They make VTK easier to use.
  2. No matter how gently it is done (it is done gently in VTK, which is nice), any API change without direct benefit to the user causes frustration.

For the same reasons the convenience methods should stay in vtkThreshold, too. To show what I mean exactly and save VTK developers some time, I’ve created a pull request that restores the mistakenly removed 3 methods and adds the missed SetThresholdFunctionTo... methods: https://gitlab.kitware.com/vtk/vtk/-/merge_requests/9709

Note that I don’t argue for reverting all ongoing vtkThreshold API changes - most of them make sense (use SetInputArrayToProcess instead of SetAttributeModeToDefault etc.). I also agree that VTK’s deprecation process is nice, there is not much to improve there. I’m just aiming for keeping VTK users happy by preserving convenience of VTK API and minimizing API churn.

1 Like
11

The methods you point out are different way to change the same property, which is fine. VTK has a lot of redundant API as you say so and it is practical.

The problem with the threshold API is that it changed multiple properties with a single call.

If adding these methods make the API better to use, then why not add a ThresholdByUpperWithComponentUseAll, a ThresholdByUpperWithComponentUseAny and a ThresholdByUpperWithComponentUseSelected ? This is the same idea just applied to other properties.

But we clearly do not want that.

As for adding them back, it’s too late imo. The methods have been deprecated and removed.

12

The API was designed by smart people. They did not combine setting of multiple properties randomly, but recognized that the thresholding function and the threshold (lower, upper, or both) must be always set together, consistently, usually at the same time.

It is not too late. Most projects don’t step through every single VTK versions but only update time to time. If the API is broken in only a few VTK versions then there is a high chance that people will not run into it.

Fixing it is as easy as merging the merge request.

13

The first example @lassoan gave was a convenience API to set two properties at once (XResolution and YResolution).

I also see no reason to remove this convenience API.

Having convenience API that sets multiple properties at once is a pretty common thing. E.g. I can think of examples in Qt API where you can set left/right/top/bottom margins individually or all at the same time.

If we forget what is “non-VTK” for a while and think of what you want VTK to be, what is so inherently bad with this API that you want to break applications?

We also don’t upgrade VTK at every or even every second minor version in our project. We are still at 8.2 and planning to move to 9.x some time next year, if time permits.

1 Like
14

I just had a grep and we use ThresholdBetween in multiple places in our code. You still have time to save us from this API churn. In doing so, you may save N ྾ PortingEffort for the N users of VTK who are in our situation.

1 Like
15

Hi,

Looks like I may have incorrectly explained my point.
What I mean to say is that sometimes a property can be split in multiple parts, then it definitely makes sense to have multiple ways to set it. But two different, unrelated, properties should not be related API wise.

eg:

A filter has two properties, Foo and Bar.
Foo is an int[3] that can be decomposed in Foo1, Foo2 and Foo3.
Bar is only a string property.

The expected API would look like this:

SetFoo(int* foo);
SetFoo(int foo1, int foo2, int foo3);
SetFoo1(int)
SetFoo2(int)
SetFoo3(int)

SetBar(const char*)

What would be not expected would be:

SetFooAndBar(int*, const char*);

I hope we can agree on this.

If that is the case, then we can discuss about the vtkThreshold specifically and if ThresholdFunction and ThresholdValue can be considered two different properties or a single property that is can be decomposed in multiple parts.

I believe that it is indeed two properties.
@lassoan thinks that it is a single property and that it should be possible to set it together.

16

I understand that you feel that minimum and maximum values and the threshold function should be rather treated as three independent properties. However, the ThresholdBetween/ThresholdByUpper/ThresholdByLower API has been around for more than 25 years and it is still used in 9 other thresholding filters in VTK; and thousands of VTK-based projects rely on them.

To summarize, what I’m hoping to achieve by keep spending time with this conversation are:

  1. Revert this unnecessary breaking API change (by merging https://gitlab.kitware.com/vtk/vtk/-/merge_requests/9709) to save thousands of VTK users from some frustration and wasted time. The API change could cost the open-source community $100-200k effort that could be better spent elsewhere.
Cost estimation of vtkThreshold API change in open-source projects

Sampling of usage of ThresholdBetween on GitHub indicates that this change would impact thousands of projects. Assuming one hour workload to manage this API change per project (find the root cause of the issue - non-trivial in Python code, because the problem does not come up at compile time; implement a solution that works for both current and older VTK versions, test the solution, in a few years remove the backward-compatibility code) the damage to the open-source community alone is about $100-200k. This money could be spent on much better things then managing unnecessary API churn. To see some of the noise that the deprecation has started to cause see these recent issues.

  1. Make sure that in the future, breaking changes are introduced into VTK API only with a very good reason. Maybe some rules could be established and described in the VTK coding guide that would help making well balanced decisions.

Since this discussion does not seem to converge to a solution. I would appreciate if other VTK core developers could share their insights. Maybe @ken-martin or @will.schroeder? Thank you!

1 Like
17

I see no harm restoring the previous API and agree with the benefits.

18

I suppose there is another way to look at this.
If for example vtkThreshold contained a vtkRange object

class vtkRange
{
private:
  int lower;
  int upper;
}

then it would seem reasonable to have
vtkThreshold::SetRange(const vtkRange& value)
VtkThreshold::SetRange(int lower, int upper)

19

I also think the old method should be restored. The arguments against are straw men.

20

@lassoan : Great insight about evaluating the impact using github directly. Very interesting approach. Also reading the issues you sentiment seems to be definitely shared between VTK users. One of note from a pyvista user (context):

We wouldn’t blame you for being ruthlessly annoyed by the continual deprecation churn from Vtk. We kinda are. Like, seriously. Vtk, can you just slow down that moving API target a little there? face_exhaling

Looks like there is an agreement about the usefullness of this API, so I will move forward with the restoration of the API.

That being said, this should have been caught much earlier imo in order to avoid forcing VTK users to change their use of VTK.

@lassoan , it looks like @jcfr and yourself fixed that in slicer in June, what made you open this thread now and not at the time ? I’m just trying to understand what I did wrong here (apart misdjuging the usefulness of an API).