Refactor OpBuilder to improve usability#74
Conversation
ctrueden
force-pushed
the
scijava/scijava-ops-api/op-builder-refactor
branch
from
53ae8a6 to
f77076b
Compare
There was a problem hiding this comment.
@hinerm Thanks a lot for working on this! I wrote my thoughts as I went through the commits. Aside from tweaks to javadoc in various places, my big question is whether we should collapse the op(String) and arityX() calls into one layer. A secondary implied question is whether to use the terms nullary, unary, and binary instead of arity0, arity1, and arity2 for those methods.
P.S. I force-pushed the branch to have one more leading commit, which I've already pushed to main: an update to pom-scijava 34.1.0. This eliminates 12500 lines of build log from the repeatedly echoed active profiles.
|
@hinerm thank you for making these changes. I really appreciate the work you did on the javadoc. That was needed. I still do not see the value in adding these I feel like for a long time I've been saying that I don't see the value in autocomplete in Ops, hence not bothering to fix it, so maybe I'll try to spill out my thinking below: My intuition says that if you know a function that does what you want to do, then you should just call that function directly. I see Ops as a way to help people who don't know what function to call. By using Ops to call a specific function, you'll take a performance hit due to matching, parameter conversion, etc. So, if you're using Ops to find some unknown function matching your needs, I only see autocomplete as being useful if it can tell you:
The OpBuilder syntax can't do either of these things. With that said, what information do I gain from autocomplete? The only thing that I can think of is that you can see that for the Again using my intuition above, I really envision Ops being most useful when you don't have these smart IDE features telling you about the functions in your environment, like with PyImageJ or in Fiji's Script Editor. The OpBuilder is a solution to this problem, promising you that it will try its best to find a function you can use to fulfill your task. Writing At the end of the day I'm probably bikeshedding, hence my approval if you guys really like this change, but I still think that we are focusing on the wrong place here. |
Ah interesting. Amusingly this is not the same as how I see Ops. 😆 So not surprising that there are different opinions here. I would expect Ops to be used when you know the algorithm you want, but don't know what the inputs will be (and/or want to future-proof for unknown future inputs). The cost of time and knowledge complexity is paid for by the fact that core code (calling the algorithms) doesn't have to change for future use-cases to evolve. Based on past discussions, I don't know that the
I think these use cases will be great and will use the enhanced pythonic dynamic entry points - but those things are in some obfuscated future. I don't think they will use the So with that in mind, the I don't want to portray this as "just for auto-complete," although I know that's how I first brought it up in this issue. Auto-complete just illustrated how many things the user has to choose between. The reality was, when I first was using the
This is totally valid. I feel the same way. I don't like having a step to select arity. I don't want to think about arity at all. I want an Different developers will probably find one option fits them better than the other. Having 2 input steps feels more comfortable to me, but ultimately if the |
|
@gselzer wrote:
The downside to calling a function directly is that you lose the extensibility provided by Ops. You will no longer gain potential performance benefits for more specialized algorithm implementations. I agree with @hinerm's point here:
With emphasis on the "want to future-proof for unknown future inputs" and especially keeping the door open for more performance-optimized implementations, as highlighted in Ops design goal 3: "Very fast". @hinerm wrote:
Here I disagree; I think the niche of the op builder is for developers programming in Java, who want type safety. There will continue to be some people programming in Java.
Indeed. I think this is a nice way to summarize the benefit of splitting out the arity layer.
☝️ To me, this is the most crucial point. @gselzer and I cannot use the builder from a new developer's perspective, whereas @hinerm had that window of opportunity, and observed a point of confusion, and identified a way to relieve it. Therefore, I accept that the change is valuable. |
hinerm
force-pushed
the
scijava/scijava-ops-api/op-builder-refactor
branch
from
2eb63d7 to
f1bea64
Compare
hinerm
force-pushed
the
scijava/scijava-ops-api/op-builder-refactor
branch
3 times, most recently
from
a635409 to
d88f779
Compare
Instead of having a single choice for input/inType for all 16 arities (a total of 49 methods), we make the first choice arity 0-16 and then narrow to the 3 input/inType choices. Although this makes more method calls in the builder chain, the hope is to avoid overwhelming developers with large numbers of similar method calls.
This only affects static utility methods and makes it so that each is named uniquely, instead of having 16 "matchInplace1" methods.
If the full line is conditional, the #end should appear on the following line (so the conditional line includes a newline character). If only the final part of the line is conditional, so that the #end appears at the end of the line, a standalone line-break must appear after the #end. This looks unintuitive in the template, but is the only way I could find to avoid Velocity eating the newline otherwise. Inline conditionals that do not appear at the end of the line are unaffected, as the newline character is not enclosed in the if..else.
This is clearer that a match is happening when this method is called.
Specifically the generic parameter of the given Nil
These steps should all now: * have @return statement javadoc * state that this function is performing a match
The changed functions are intermediate builder steps and do not actually perform a match at this point.
All the "@see" statements should now have explanations as to why one would use these other methods in place of the current method.
These locations that used to have a terminal #end no longer do, with the addition of "@see" explanations.
These Functions will return Objects since the output is unknown
Specifying the Hints when initially creating an OpBuilder provides the potential for simplification when reusing that builder, while also cutting the number of terminal steps in half (since they do not need to be overloaded with ...Hints signatures)
Since this is the entry point for actually using the builder it makes more sense to document there than on the class.
Specifying the "arity1, 2, 3, etc..." part of the builder is a pain if you are more familiar with ops. By adding convenience methods with common "arity" names (e.g. "unary", "binary", etc..) bypasses the need to make this extra call within the builder process. Note that this is not perfect, as changing the number of arguments will still necesitate a change to the "arity" portion of the builder call. But this is a compromise. Note that the moving of "Hints" to OpEnvironment coupled with this change now means there are 32 new signatures.
|
Thanks again for all your work on this, @hinerm! Earlier, we had discussed potentially compacting the ops.arity3("logic.ternary").input(condition, trueValue, falseValue).apply()
ops.binary("math.add", hints).input(img1, img2).output(result).compute()etc. What is the benefit of keeping the ops.binary("math.add").hints(hints).input(img1, img2).output(result).compute()so that the @hinerm If you like the idea of collapsing the Alternately: we could loop in @tpietzsch and @tinevez next week to see which form they find the best. What do you think? |
|
|
Awesome, sorry I missed that! I saw that the builder usages across the codebase were still using the long-form |
I think we might want to do another separate pass on |
yeah I did the changes in a dumb order 🙄 |
3 participants
The main API changes include:
arityN()step in the builder, after specifying Op name but before input value/type(s). This avoids thearity * inputMethodsmethod proliferation.matchInplacestatic utility methods now include two digits, indicating both the arity and the mutated input. This avoids having to try and count the desired number of total inputs when selecting between overloadedmutateInplace1methods.All public API should now have javadoc. It should be fairly consistent across similar methods. Some particular areas of attention:
inplace/mutatemethods just@seeto the first arity, while each individualinplace/mutatemethod@see's to the complementary aritymutate/inplace. Alternatively we could link to ALL the other arities in one or both of these location(s). Or we could make a single dump of all methods in the class javadoc... but given the nature of the builders I think the method javadoc is much more visible.Other thoughts:
I was considering expanding the
[I/O][T/V/U]acronyms in some abbreviated way. I initially found them confusing and didn't like the "single-letter differentiation". After looking at the builder for a while now I'm pretty used to them, and hopefully the javadoc is clear what decision you're making at each point. So I decided to just define the acronyms in the class javadoc.Currently a draft because of the API changes. @gselzer @ctrueden if you're ok with these changes I will update the builder uses throughout the incubator. No rush; just let me know!