reverse deprecation on abs() and fabs() about stanc3 HOT 20 CLOSED

I think we need to make a decision on this once and for all. It used to cause us huge headaches before we got better at controlling C++ signatures.

I like the idea of keeping fabs,fmax and fmin because they follow the C++ spec. But I also like the idea of extending abs so that it works the way everyone coming from other languages would expect.

from stanc3.

@syclik we fixed with stan-dev/stan#1649 which was merged, ok to close?

from stanc3.

yes.

from stanc3.

I think we can solve this one and it'll be less confusing to users not familiar with the C++ function fabs.

from stanc3.

Currently in the language we have the following abs() functions:

int abs(int)
real abs(real)

and the following for fabs():

real fabs(int)
real fabs(real)
vector fabs(vector)
row_vector fabs(row_vector)
matrix fabs(matrix)
real[] fabs(int[])
real[] fabs(real[])
vector[] fabs(vector[])
row_vector[] fabs(row_vector[])
matrix[] fabs(matrix[])
real[,] fabs(int[,])
real[,] fabs(real[,])
vector[,] fabs(vector[,])
row_vector[,] fabs(row_vector[,])
matrix[,] fabs(matrix[,])
real[,,] fabs(int[,,])
real[,,] fabs(real[,,])
vector[,,] fabs(vector[,,])
row_vector[,,] fabs(row_vector[,,])
matrix[,,] fabs(matrix[,,])
real[,,,] fabs(int[,,,])
real[,,,] fabs(real[,,,])
vector[,,,] fabs(vector[,,,])
row_vector[,,,] fabs(row_vector[,,,])
matrix[,,,] fabs(matrix[,,,])
real[,,,,] fabs(int[,,,,])
real[,,,,] fabs(real[,,,,])
vector[,,,,] fabs(vector[,,,,])
row_vector[,,,,] fabs(row_vector[,,,,])
matrix[,,,,] fabs(matrix[,,,,])
real[,,,,,] fabs(int[,,,,,])
real[,,,,,] fabs(real[,,,,,])
vector[,,,,,] fabs(vector[,,,,,])
row_vector[,,,,,] fabs(row_vector[,,,,,])
matrix[,,,,,] fabs(matrix[,,,,,])
real[,,,,,,] fabs(int[,,,,,,])
real[,,,,,,] fabs(real[,,,,,,])
vector[,,,,,,] fabs(vector[,,,,,,])
row_vector[,,,,,,] fabs(row_vector[,,,,,,])
matrix[,,,,,,] fabs(matrix[,,,,,,])

Is this still relevant and we want to deprecate fabs?

from stanc3.

I second this sentiment, decisions are always a good thing. I favor extending abs to have an uniform specification.

Having an uniform specification is much better than adhering to the C++ constructs. There are pros to adhering to C++, for example if someone wants to tweak the functions it provides a consistent framework. However, Stan is sophisticated as a higher level domain specific language and the use case is perhaps a little different.

from stanc3.

from stanc3.

To resurrect this issue, do we want to make abs have all the same signatures as fabs? Should we also add int container signatures abs(array[] int): array[] int etc?

from stanc3.

Do we want to make abs have all the same signatures as fabs?

No. The fundamental problem is the conflict between:

real fabs(int)

and

int abs(int)

So the question is whether we can get away without the fabs version? If so, we can just go back to abs everywhere.

We now also have

complex abs(complex)

from stanc3.

I believe our type promotion is strong enough for that to be valid everywhere. If it is not, that would itself be a bug.

I guess that means we are not really talking about deprecating fabs, just un-deprecating abs?

from stanc3.

I think the right thing to do is deprecate fabs because we don't need real fabs(int) as a signature. If we only have int abs(int) the promotion should be fine and there are no derivatives to track.

One issue is that int abs(int) won't fit into our vectorization scheme.

from stanc3.

So we would be deprecating fabs but only for int arguments? Even if we eventually removed that deprecation, type promotion means you could still always call fabs with an int, right?

from stanc3.

I really meant just flat out deprecating all signatures for fabs and going with abs. I don't see any reason we'd need a real fabs(int).

That means implementing signatures for abs:

int[...] abs(int[...]);
real[...] abs(real[...]);
vector[...] abs(vector[...]);
row_vector abs(row_vector[...]);
matrix[...] abs(matrix[...]);

where the ... indicates zero or more indexes. All of these other than the int one are implemented for fabs, so we can delegate to those for cases other than integer arrays, which will need a new implementation.

It's still not quite a drop-in replacement, because we currently have

real[...] fabs(int[...])

but int[...] isn't assignable to real[...] everywhere yet.

from stanc3.

That is what I broadly meant by "do we want to make abs have all the same signatures as fabs", with the caveat of the int container case. That has to be done on the stan-math side, right?

from stanc3.

Yes, the behavior of abs and fabs are the same for non-integer args. And yes, that has to be done on the Stan math side. It's a pretty easy change on the math side because the int signatures don't need autodiff tests.

from stanc3.

We either fix this in stan-math or make a special case so that abs is generated with fabs in C++. Though I am not sure I like it.

from stanc3.

I am not sure I like it.

Which part don't you like---removing fabs or implementing abs for continuous args by delegation to fabs?

from stanc3.

Ah sorry, just read again what I wrote, sorry for being confusing.

I dont like the latter option of the two I wrote: “special casing in stanc3 so that abs is generated as fabs”. This is how we deal with lmultiply/multiply_log and would rather see a fix in Stan Math.

from stanc3.

I created the supporting issue in the math lib. Might as well knock off lmultiply/multiply_log while we're at it: stan-dev/math#2595

from stanc3.

Since this has last been discussed, @bob-carpenter's comment on why we should do this yet:

but int[...] isn't assignable to real[...] everywhere yet.

has become untrue - Stan containers are now fully covariant.

Therefore, it is safe to do this deprecation and expose
T abs(T x) where T is any Stan type including nested arrays.

@bob-carpenter is looking into the C++ side to make sure the vectorization is implemented, but as soon as the math library is good to go on this we will be deprecating fabs and exposing all of the signatures under abs

from stanc3.