Change argument order of NonlinearOperator

[blegat]

This also changes the order between the two because when one of the two has a default value depending on the other one it's usually the second positional argument.
It also allows to write NonlinearOperator(func::Function, head::Symbol = Symbol(func)) in the docstring which wouldn't be possible if the order was reversed.
This change only matters when NonlinearOperator is called directly and not through @operator.
It seems to me that when NonlinearOperator is called directly, you would want the symbol to be the function name 99% of the time so having this default value makes sense. It's also the default in the @operator macro so it's consistent.

[odow]

I'm not in favor of this.

Adding lots of shortcuts for the sake of saving a few characters typing is not worth the trade-off. Constructing new operators with the constructor will be used very rarely.

Also, we shouldn't be advising people to use the NonlinearOperator constructor. Users should interact with it via @operator and add_nonlinear_operator.

If needed at all, surely this just requires the method:

NonlinearOperator(f::Function) = NonlinearOperator(Symbol(f), f)

[odow]

I also think that, as a rule, we should be considering only breaking changes right now. Quality of life changes that can be added as feature additions can wait until this is out in the wild and we see how people are using it.

[blegat]

It's breaking if we reorder the fields.

Constructing new operators with the constructor will be used very rarely.
Also, we shouldn't be advising people to use the NonlinearOperator constructor. Users should interact with it via @operator and add_nonlinear_operator.

I think that's where we disagree. If you're not planning to set the MOI.UserDefinedFunction attribute then the add_nonlinear_operator function is a bit combersome since you need to give the model and the arity which prevents you to define an operator that could be used with any model and the arity doesn't matter here.

[odow]

It's breaking if we reorder the fields.

Why do we need to do that?

I think that's where we disagree.

Who is going to be using NonlinearOperator? The only use-case I can see would be inside DCP.jl, and that can just type out the symbol. It doesn't need to save space.

[odow]

I guess swapping the order is in the style guide, which says that functions should go as the first argument.

[codecov]

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (a573de2) 98.09% compared to head (e483105) 98.09%.
Report is 3 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #3468   +/-   ##
=======================================
  Coverage   98.09%   98.09%           
=======================================
  Files          37       37           
  Lines        5501     5501           
=======================================
  Hits         5396     5396           
  Misses        105      105           

Files Changed	Coverage Δ
src/macros.jl	97.97% <ø> (ø)
src/nlp_expr.jl	99.24% <100.00%> (ø)

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

[odow]

I'm okay with this. But I don't think we need an optional argument.

[blegat]

Who is going to be using NonlinearOperator? The only use-case I can see would be inside DCP.jl, and that can just type out the symbol. It doesn't need to save space.

It's not about saving the space, it's about removing the possibility to make a typo where you miss a character in the symbol for instance. Again, any non-AD usage would require that because we hardcode the list of operators to be exactly AD. Since we have MOI.ListOfSupportedNonlinearOperator in MOI, it means we expect solver to be able support more (e.g., Alpine, DCP, constraint programming, ...). These solvers would want to define op_foo for some foo function and would want it to look like foo because how the expressions was constructed doesn't matter. Also, if it's build front MOI, op_foo isn't even a thing so there is no reason to name it something else than foo.