min max for double

[Marco Inacio]

Pardon if my mistake, but is there any reason for the fact that
min(double, double) and max(double, double) are not in grammar, but
max(int, int) and min(int, int) are?

no matches for function name="min"
arg 0 type=real
arg 1 type=real
available function signatures for min:
0. min(int[1]) : int
1. min(real[1]) : real
2. min(vector) : real
3. min(row vector) : real
4. min(matrix) : real
5. min(int, int) : int

(This is consistent with the manual)

Also, oddly, max(a, b) and min(a, b) functions are actually defined for
doubles only:

namespace stan {
namespace math {
inline double max(const double a, const double b) {
return a > b ? a : b;
}
}
}

[Bob Carpenter]

Yes, we could add this, but it's a bit subtle to get the
signatures right for promotion. The C++ lib and Stan as it
is now only include these two signatures:

int max(int,int);

double fmax(double,double);

This allows calls to

double fmax(double,int);
double fmax(int,double);
double fmax(int,int);

You'll find it a bit tricky to get the promotions right.
We need to add:

double max(double,double);
double max(int,double);
double max(double,int);
int max(int,int);

I think that'll keep us out of trouble with attempts to
promote (demote) to the int type from double.

- Bob

[Marco Inacio]

On 13-05-27 07:34 PM, Bob Carpenter wrote:

Yes, we could add this, but it's a bit subtle to get the
signatures right for promotion.  The C++ lib and Stan as it
is now only include these two signatures:

   int max(int,int);

   double fmax(double,double);

This allows calls to

   double fmax(double,int);
   double fmax(int,double);
   double fmax(int,int);

You'll find it a bit tricky to get the promotions right.
We need to add:

   double max(double,double);
   double max(int,double);
   double max(double,int);
   int max(int,int);

I think that'll keep us out of trouble with attempts to
promote (demote) to the int type from double.

I created two functions:

boost::math::tools::promote_args<T1,T2>::type max(T1 a, T2 b)

and a specialized version:

int max(int a, int b)

They passed the tests to check types according to what you specified:

https://github.com/randommm/stan/commit/dff2eb85ce35d32552194ea2002b9e66c72e0fa9

[Bob Carpenter]

Looks right to me.

- Bob

> --
> You received this message because you are subscribed to the Google Groups "stan development mailing list" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to stan-dev+u...@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
>
>

[Marco Inacio]

Then should I specialize them for var/fvar? Currently, no version of
min/max has specializations for var/fvar, and I really find this reasonable.

But fmax/fmin does:

inline var fmax(const stan::agrad::var& a,
const stan::agrad::var& b) {
return a.vi_->val_ > b.vi_->val_ ? a : b;
}


I find this code strange:

inline var fmax(const stan::agrad::var& a,
const double& b) {
return a.vi_->val_ >= b ? a : var(b);
}

This seems to be a odd case to arise, but if it does, then the gradient
will be carried on if it's greater, otherwise a new var is started, but
is this really a wanted behavior? Or is it better always return a new
var, even if b is higher?

[Bob Carpenter]

No, they don't need specializations.

On 5/29/13 1:28 AM, Marco Inacio wrote:
> Then should I specialize them for var/fvar?

> Currently, no version of min/max has specializations for var/fvar, and I
> really find this reasonable.

Yes, they're only defined for int. But if we change that,
they'll need to follow fmax.

>
> But fmax/fmin does:
>
> inline var fmax(const stan::agrad::var& a,
> const stan::agrad::var& b) {
> return a.vi_->val_ > b.vi_->val_ ? a : b;
> }

Now that you bring it up, we could return a const reference:

inline var&
fmax(const var& a, const var& b) {
if (a > b)
return a;
else
return b;
}

I'm not sure if you can take a reference through the conditional
operator (a > b ? a : b).

> I find this code strange:
>
> inline var fmax(const stan::agrad::var& a,
> const double& b) {
> return a.vi_->val_ >= b ? a : var(b);
> }
>
> This seems to be a odd case to arise, but if it does, then the gradient will be carried on if it's greater, otherwise a
> new var is started, but is this really a wanted behavior? Or is it better always return a new var, even if b is higher?

We need to support all the polymorphism that's possible
in Stan, so we need to allow this.

You're right about the behavior of the gradient. I
don't see any other alternative. As is, if you have two var inputs,
you lose the gradient info on one of them anyway.

We could try to warn users in the doc. It gets hairy
to start issuing warnings on a per-function, per-argument config
basis.

- Bob

[Marco Inacio]

Then what to do with fmin/fmax, there seems to be some confusion even in
the manual: page 145 incorrectly uses fmax/fmin for vectors (invalid code).

If min/max is going to cover what they do, then, maybe it would be
better to place them as deprecated and warn the users to use min/max always.

[Bob Carpenter]

min/max won't quite cover what fmin/fmax does because
the return type is different for (int,int) inputs.
But that should be totally OK everywhere in Stan.

Thanks for pointing out the error in the manual. If you
add these functions, would you mind fixing the manual why
you're at it? Or better yet, adding the vectorized versions!

This is the problematic fragment:

data {
int<lower=1> N;
real y[N];
}
parameters {
real<lower=fmin(y),upper=fmax(y)> phi;
}

I think we need an easier way to update the manual without
making pull requests! And we should be testing all of
these fragments (I wrote most of it, so this is my bad).

- Bob

[Ben Goodrich]

On Thursday, May 30, 2013 5:14:08 PM UTC-4, Bob Carpenter wrote:

I think we need an easier way to update the manual without
making pull requests!  And we should be testing all of
these fragments (I wrote most of it, so this is my bad).

I agree that a change to the manual only should only have to satisfy make manual.

I think it would be better if the .stan code blocks were each separate files that were put into the manual with a \verbatiminput

http://www.tex.ac.uk/cgi-bin/texfaq2html?label=verbfile

That way, we could have a make target to verify that they all parsed (and perhaps compiled). However, many of the .stan code blocks aren't parsable because they have various informal things like ..., DO SOMETHING, etc. So, it would take a good bit of tedious work to get them all into shape. But that would be better than having a misleading manual.

Ben

[Bob Carpenter]

That's what I did for our manuals at our company.

The challenge is that they're not whole models, just
fragments. They could be embedded in whole models, but
those shouldn't go in the manual.

I have some pretty robust Java code that I'm happy to share
that cuts out bits of models and includes those. It uses code
comments to indicate where to start/stop and what the name of
the resulting file to include is. Something like:

parameters {
real a;
}
model {
/* start block.tex */
a ~ normal(0,1);
/* end */
}

would print just

a ~ normal(0,1);

I'd probably have to beef it up for some of our special symbols.

Another nice part about it is that it includes only half vertical
lines for empty lines, which helps with clarity without wasting
ridiculous amounts of vertical space.

The only issue is that it'd introduce a Java dependency on making
the manual. And make things that much more complex for everyone
else to understand.

- Bob

[Ben Goodrich]

On Thursday, May 30, 2013 6:07:40 PM UTC-4, Bob Carpenter wrote:

That's what I did for our manuals at our company.

The challenge is that they're not whole models, just
fragments.  They could be embedded in whole models, but
those shouldn't go in the manual.

Maybe we could have two subdirectories under src/doc/stan-reference/ like models/ and fragments/ where every file under fragment/ was part of a .stan file under models/. Then we can test that everything under models/ parses and use the \verbatiminclude statements to pull in things from fragments/. The fragmentation could be done with sed (which is used in the makefiles anyway) scripting to avoid the java dependency.

Ben

[Bob Carpenter]

That's exactly how my Java package works. We'd just need to
rewrite it with awk/sed (or do we not have awk, either?). I
don't know sed (or awk for that matter).

- Bob

[Michael Betancourt]

I do! #volunteeringwithamillionitherthingstodo

[Ben Goodrich]

I don't think we need awk. Although sed is complicated / powerful and there are some differences between GNU sed and BSD sed, extracting text between two anchors is not so hard

http://stackoverflow.com/questions/6127786/how-to-extract-text-between-two-words-in-unix

Ben

[Marco Inacio]

On 13-05-30 06:14 PM, Bob Carpenter wrote:
> min/max won't quite cover what fmin/fmax does because
> the return type is different for (int,int) inputs.

I don't get the problem here. After I coded it, I made sure that
min(int, int) will return int.

And that it'll return double for (int, double), (double, int) and
(double, double). Is the problem related to fvar which is more
problematic to promote?

> But that should be totally OK everywhere in Stan.
>
> Thanks for pointing out the error in the manual. If you
> add these functions, would you mind fixing the manual why
> you're at it? Or better yet, adding the vectorized versions!

I forgot to mention, I already fixed the code at this page in my fork,
it'll go together with the pull request. I haven't found other
problematic pages related to max/min, but I'll check again later, and
also check places where vectorization is possible.

[Bob Carpenter]

On 5/30/13 7:21 PM, Marco Inacio wrote:
>
> On 13-05-30 06:14 PM, Bob Carpenter wrote:
>> min/max won't quite cover what fmin/fmax does because
>> the return type is different for (int,int) inputs.
>
> I don't get the problem here. After I coded it, I made sure that min(int, int) will return int.

fmax(int,int) returns double, whereas max(int,int) returns
an int. Given that the int is guaranteed to be in range and
ints can be cast to double in Stan, there's no problem.

> And that it'll return double for (int, double), (double, int) and (double, double). Is the problem related to fvar which
> is more problematic to promote?
>
>
>> But that should be totally OK everywhere in Stan.
>>
>> Thanks for pointing out the error in the manual. If you
>> add these functions, would you mind fixing the manual why
>> you're at it? Or better yet, adding the vectorized versions!
> I forgot to mention, I already fixed the code at this page in my fork, it'll go together with the pull request. I
> haven't found other problematic pages related to max/min, but I'll check again later, and also check places where
> vectorization is possible.

Any bug reports are hugely appreciated, but we're going
to be taking a higher-level approach to make sure all our
code runs. And we can probably put one of our summer interns
to work starting on making sure everything in the manual runs.

- Bob

[Marco Inacio]

> fmax(int,int) returns double, whereas max(int,int) returns
> an int. Given that the int is guaranteed to be in range and
> ints can be cast to double in Stan, there's no problem.

Sorry to insist on this point, but why then have a function with
different name just to return a double instead of a int since an int
would be accepted anywhere doubles are via implicit casted/promoted to
double? (or this idea is wrong?)

Something similar happens with fabs() and abs():

abs() and fabs() are a confusing case for the user since they are
definied together at documentation exactly the same way, with the same
words and there's no explanation on where one should use each one.

real abs(real x)
The absolute value of x
real fabs(real x)
The absolute value of x
(page 199)

[Bob Carpenter]

On 5/31/13 12:14 AM, Marco Inacio wrote:
>
>> fmax(int,int) returns double, whereas max(int,int) returns
>> an int. Given that the int is guaranteed to be in range and
>> ints can be cast to double in Stan, there's no problem.
>
> Sorry to insist on this point, but why then have a function with different name just to return a double instead of a int
> since an int would be accepted anywhere doubles are via implicit casted/promoted to double? (or this idea is wrong?)

I think we're in agreement for Stan. (I was just pointing
out why they are different in C++ and why having the signatures
below isn't exactly equivalent to what we have now.)

Specifically, I'm OK with deprecating fmax() and generalizing max()
to only use the Stan signatures:

int max(int,int);
real max(int,real);
real max(real,int);
real max(real,real);

And similarly for min() vs. fmin().

Here's a nice discussion of the distinction in C++:

http://stackoverflow.com/questions/1632145/use-of-min-and-max-functions-in-c

We absolutely need to keep the distinction as-is in the
C++ library for C++ compatibility, but that's a different
issue than keeping it in Stan.

It's going to be tricky, though, with the way Stan compiles.
It will just use max() for max() and it will try to call
the standard libraries unless we're careful to import our own
and our own have better type matches.

> Something similar happens with fabs() and abs():
>
> abs() and fabs() are a confusing case for the user since they are definied together at documentation exactly the same
> way, with the same words and there's no explanation on where one should use each one.

We can more heavily overload here, too, and deprecate fabs()
and just go with these:

int abs(int);
real abs(real);

> real abs(real x)
> The absolute value of x
> real fabs(real x)
> The absolute value of x
> (page 199)

I should have never included

real abs(real);

in the first place.

- Bob

[Marco Inacio]

> Here's a nice discussion of the distinction in C++:
>
> http://stackoverflow.com/questions/1632145/use-of-min-and-max-functions-in-c
>
> We absolutely need to keep the distinction as-is in the
> C++ library for C++ compatibility, but that's a different
> issue than keeping it in Stan.

Nice SO link! Even after reading it though, I couldn't saw any problems
arriving provided there's the specialization for (int, int).

>
> It's going to be tricky, though, with the way Stan compiles.
> It will just use max() for max() and it will try to call
> the standard libraries unless we're careful to import our own
> and our own have better type matches.

I didn't know that, how does this happen? My guess was that there were a
"using std;" in Stan compiled code, but I found none, so why would it
call std::max? Is there a way I could simulate this behavior in a unit
test to check which function is being called?

> We can more heavily overload here, too, and deprecate fabs()
> and just go with these:
>
> int abs(int);
> real abs(real);

Are there other functions that already deprecated? I ask because I want
to now the procedure here. I would suggest removing the deprecated
functions from the manual and showing the user an error message asking
to use the new function and refusing to continue compilation.

[Bob Carpenter]

On 5/31/13 1:37 AM, Marco Inacio wrote:
>
>> Here's a nice discussion of the distinction in C++:
>>
>> http://stackoverflow.com/questions/1632145/use-of-min-and-max-functions-in-c
>>
>> We absolutely need to keep the distinction as-is in the
>> C++ library for C++ compatibility, but that's a different
>> issue than keeping it in Stan.

> Nice SO link! Even after reading it though, I couldn't saw any problems arriving provided there's the specialization for
> (int, int).

I don't think there is a problem for Stan.

We just have to be careful not to break the
expected C++ behavior in our API. That means leaving
the fmin and fmax functions in place and behaving as
they currently behave.

>> It's going to be tricky, though, with the way Stan compiles.
>> It will just use max() for max() and it will try to call
>> the standard libraries unless we're careful to import our own
>> and our own have better type matches.
>
> I didn't know that, how does this happen? My guess was that there were a "using std;" in Stan compiled code, but I found
> none, so why would it call std::max? Is there a way I could simulate this behavior in a unit test to check which
> function is being called?

This is very confusing in C++.

Check out this description of abs(), with signature

int abs(int);

in <cstdlib>:

http://www.cplusplus.com/reference/cstdlib/abs/

You don't need an include to compile this program in
file abs.cpp:

#include <iostream>
int main() {
std::cout << "abs(-2.3)=" << abs(-2.3) << std::endl;
}

It has this rather unexpected behavior:

~/temp2$ clang++ abs.cpp
~/temp2$ ./a.out
abs(-2.3)=2

What's happening is that the double value -2.3 is getting
cast to an integer (-2) and then run through abs().

Turns out there's another abs() in <cmath> with three
overloaded signatures. Now compare this program:

#include <cmath>
#include <iostream>
int main() {
std::cout << "abs(-2.3)=" << abs(-2.3) << std::endl;
std::cout << "std::abs(-2.3)=" << std::abs(-2.3) << std::endl;
}

which as you might expect does this:

~/temp2$ clang++ abs2.cpp
~/temp2$ ./a.out
abs(-2.3)=2
std::abs(-2.3)=2.3

An explicit "using" seems to force the std::abs version
as seen in abs3.cpp:

#include <cmath>
#include <iostream>
int main() {
using std::abs;
std::cout << "abs(-2.3)=" << abs(-2.3) << std::endl;
std::cout << "std::abs(-2.3)=" << std::abs(-2.3) << std::endl;
}

which produces this:

~/temp2$ clang++ abs3.cpp
~/temp2$ ./a.out
abs(-2.3)=2.3
std::abs(-2.3)=2.3

Good unit test should be able to catch all this if there are
different behaviors. Otherwise, it's not really unit testable.
For lgamma, the error behavior was different.

I wouldn't worry about deprecation for the moment. Feel
free to just leave fabs() and fmin() and fmax() and change the
doc to indicate they're there for C++ lib compatibility.

- Bob

[Marco Inacio]

Thanks for the explanation. I see now where the trouble comes from, it's
indeed pretty trick this heritage from C without namespaces. I'll test
this to make sure no double will be converted to int by the C++/C libs,
I just hope there's no substantial difference between compilers in this
area.

It would be cool if the parser had the ability to change min into min_
and the same for max and abs, do you find this worth implementing one
day? I know nothing about parsers, so I can't help with that.

[Bob Carpenter]

Doing replacements like this wouldn't be too hard.
It's already done on an ad hoc basis for the operators
for matrices.

We'll probably have to do something more general eventually.
That can involve namespaces (which doesn't play nicely with
argument-dependent lookup) or name mangling.

I'd rather keep things simple for now if we can.

- Bob

[Daniel Lee]

Regarding pull requests for manual changes: hopefully it doesn't slow you down too much.

I'd suggest just writing a comment saying that this affects the manual only, that you've built the manually successfully on your machine, and that it could be merged immediately. If I see that (from you), I'll merge it.

I wish there was a way to have multiple pull request builders, but I'm not trying to be a Jenkins ninja and do crazy stuff with it.

Daniel

[Ben Goodrich]

The only issue I see with that is that I don't have the magic fonts. So, it is possible that make manual succeeds on my machine but either doesn't succeed or succeed but looks wrong on a machine with the magic fonts.

[Daniel Lee]

On Fri, May 31, 2013 at 4:00 PM, Ben Goodrich <goodri...@gmail.com> wrote:

The only issue I see with that is that I don't have the magic fonts. So, it is possible that make manual succeeds on my machine but either doesn't succeed or succeed but looks wrong on a machine with the magic fonts.

I think that's ok. We don't have any automated tests that will visually look at the file. If the change ends up breaking on the Windows box, that'll tell us something and we'll catch it the next morning.

 

[Bob Carpenter]

On 5/31/13 3:57 PM, Daniel Lee wrote:
> Regarding pull requests for manual changes: hopefully it doesn't slow you down too much.
>
> I'd suggest just writing a comment saying that this affects the manual only, that you've built the manually successfully
> on your machine, and that it could be merged immediately. If I see that (from you), I'll merge it.
>
> I wish there was a way to have multiple pull request builders, but I'm not trying to be a Jenkins ninja and do crazy
> stuff with it.

This makes sense to me.

I think it also makes sense to make sure the current
version of the manual refers to the current state of
develop. So changes for new features should go in with
those features.

I was originally thinking we'd just have a single feature
thread between each release for the manual, but that doesn't
let us synch with develop easily, at least given my limited
imagination w.r.t. configuration.

- Bob