Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add 'safe_power' engine function that sets 0^0 to 0 #200

Open
bbolker opened this issue May 2, 2024 · 11 comments
Open

add 'safe_power' engine function that sets 0^0 to 0 #200

bbolker opened this issue May 2, 2024 · 11 comments
Assignees
Labels
engine Component: C++ / TMB Engine

Comments

@bbolker
Copy link
Collaborator

bbolker commented May 2, 2024

it would be convenient to have an engine function that computes x^y but always returns 0 if x==0. (We have a special case where we are computing 0^0, where C++ quite reasonably returns NaN; we know we want the answer to be 0 in this case.) I took a crack at this on a branch (with CppAD::CondExpLt), but am so far failing (perhaps because of some issue with vectorization? Not sure ...)

If this is not something @stevencarlislewalker can figure out quickly I might ask on the TMB list ...

(attn: @jfree-man)

@stevencarlislewalker
Copy link
Member

Thanks Ben. I'll checkout your branch.

@stevencarlislewalker stevencarlislewalker self-assigned this May 2, 2024
@stevencarlislewalker stevencarlislewalker added the engine Component: C++ / TMB Engine label May 2, 2024
@stevencarlislewalker stevencarlislewalker moved this to 🏗 In progress in Roadmap May 2, 2024
@bbolker
Copy link
Collaborator Author

bbolker commented May 2, 2024

PS There seems to be a similar construction at line 130 of https://kaskr.github.io/adcomp/convenience_8hpp_source.html

@stevencarlislewalker
Copy link
Member

Thanks! I'm following this here: https://www.coin-or.org/CppAD/Doc/condexp.htm

@bbolker
Copy link
Collaborator Author

bbolker commented May 2, 2024

New branch at https://github.com/canmod/macpan2/tree/safe_power2, PR

@stevencarlislewalker
Copy link
Member

Thanks Ben. I think I did it a while ago, then needed to take little people to choir and then there are tests failing from other commits in the branch. Here's the pull request I was using: #201 . But I haven't pushed my changes yet. In any case, this is what it will look like I think.

case MP2_SAFE_POWER: // SAFE_POWER
                args = args.recycle_for_bin_op();
                err_code = args.get_error_code();
                switch (err_code) {
                    case 201:
                        SetError(err_code, "The two operands do not have the same number of columns", row, table_x[row] + 1, args.all_rows(), args.all_cols(), args.all_type_ints());
                        return m;
                    case 202:
                        SetError(err_code, "The two operands do not have the same number of rows", row, table_x[row] + 1, args.all_rows(), args.all_cols(), args.all_type_ints());
                        return m;
                    case 203:
                        SetError(err_code, "The two operands do not have the same number of columns or rows", row, table_x[row] + 1, args.all_rows(), args.all_cols(), args.all_type_ints());
                        return m;
                }
                m1 = args[0];
                m2 = args[1];
                rows = m1.rows();
                cols = m1.cols();
                m = matrix<Type>::Zero(rows, cols);
                for (int i; i < rows; i++) {
                    for (int j; j < cols; j++) {
                        m.coeffRef(i, j) = CppAD::CondExpEq(
                            m1.coeff(i, j), 
                            m2.coeff(i, j), 
                            Type(0), 
                            pow(m1.coeff(i, j), m2.coeff(i, j))
                        );
                    }
                }
                return m;

@stevencarlislewalker
Copy link
Member

Looks like CondExpLe is more appropriate.

@bbolker
Copy link
Collaborator Author

bbolker commented May 2, 2024

Why? In this case I don't think we want to be more permissive (i.e. safe_power(x,y) should be NaN for x<0)? #202 is the correct PR, I've added your code to it ...

@stevencarlislewalker
Copy link
Member

I now have it working with tests, and was about to check it in when I realized that R itself seems to be doing ifelse(y == 0, 1, x^y). I'll check in my changes to the branch as they are (i.e. ifelse(x == 0, 0, x^y)`) but let me know if you think we should switch.

@stevencarlislewalker
Copy link
Member

stevencarlislewalker commented May 3, 2024

More confusion. I've now asked the engine to do the case in the description of this ticket, and I get the following.

> engine_eval(~0^0)
     [,1]
[1,]    1

This is not a NaN. Anyways, I'm still going to check in to the branch, but we should continue this discussion.

@stevencarlislewalker
Copy link
Member

Another example of R doing something that my current safe_power doesn't emulate.

> matrix(0^(-2:2))
     [,1]
[1,]  Inf
[2,]  Inf
[3,]    1
[4,]    0
[5,]    0

stevencarlislewalker added a commit that referenced this issue May 3, 2024
@stevencarlislewalker
Copy link
Member

Here is the first version of the test of safe_power.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
engine Component: C++ / TMB Engine
Projects
Status: 🏗 In progress
Development

No branches or pull requests

2 participants