Open stonylohr opened 4 years ago
SGTM
@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.
@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.
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...