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

changes to consider for atan2d #8

Open
stonylohr opened this issue Jul 17, 2020 · 3 comments
Open

changes to consider for atan2d #8

stonylohr opened this issue Jul 17, 2020 · 3 comments

Comments

@stonylohr
Copy link
Contributor

stonylohr commented Jul 17, 2020

Use mutable arguments rather than shadowing.
Use std::mem::swap to swap "x" and "y".
Integer "q" instead of f64.
"match" instead of "if/else-if" when checking "q".
Borrow comments from c++ counterpart.

Putting this all together, it might look something like...

pub fn atan2d(mut y: f64, mut x: f64) -> f64 {
    // In order to minimize round-off errors, this function rearranges the
    // arguments so that result of atan2 is in the range [-pi/4, pi/4] before
    // converting it to degrees and mapping the result to the correct
    // quadrant.
    let mut q = if y.abs() > x.abs() {
        std::mem::swap(&mut x, &mut y);
        2
    } else {
        0
    };
    if x < 0.0 {
        q += 1;
        x = -x;
    }
    let ang = match q {
        // Note that atan2d(-0.0, 1.0) will return -0.  However, we expect that
        // atan2d will not be called with y = -0.  If need be, include
        //
        //   case 0: ang = 0 + ang; break;
        //
        // and handle mpfr as in AngRound.
        1 => (if y >= 0.0 { 180.0 } else { -180.0 }) - y.atan2(x).to_degrees(),
        2 => 90.0 - y.atan2(x).to_degrees(),
        3 => -90.0 + y.atan2(x).to_degrees(),
        _ => y.atan2(x).to_degrees()
    };
    ang
}
@michaelkirk
Copy link
Member

SGTM

@stonylohr
Copy link
Contributor Author

@michaelkirk I would be happy to prepare a PR for this and the other geomath items if you like: #1, #2, #3, #4, #5, #6, #7, #9, #10, and optionally the geomath parts of #11 and #12. I could break them into multiple PRs if you like.

Alternatively, I could set these aside for the possibly-distant future and instead turn to the unit test failures that came up in the recent CI additions. I see the geomath changes as likely quick, but they won't change returned values (geomath is already ok on that front), and some earlier experiments suggest to me that they'll result in at most a small performance boost, likely small enough that it will be hard to verify given normal performance variability from one bench run to the next. Basically, I currently see these as mostly making the code cleaner and more idiomatic.

So far, performance variation on benches seems to be around plus or minus 5% for me, which is disappointingly high. I've been shutting down everything except a single terminal, as far as user-presenting applications, but I haven't gone hunting for things in the background. I'd be curious to hear what variability you see, and if you come up with strategies for minimizing it.

I'll plan to start submitting issues based on the unit test failures until I hear back on your preferred approach.

@michaelkirk
Copy link
Member

@stonylohr - please feel free to work on which tasks you prefer. I'm of course most interested in the correctness issues.

As long as it's under a couple hundred lines, I should be able to review it pretty promptly, especially since we have CI hooked up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants