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

Integrate finitediff into argmin monorepo #479

Merged
merged 5 commits into from
Mar 5, 2024

Conversation

stefan-k
Copy link
Member

@stefan-k stefan-k commented Mar 1, 2024

  • Started integrating finitediff into argmin monorepo
  • Added CI pipeline for finitediff, fixed doc test
  • Updated finitediff Cargo.toml
  • Fixed ndarray warning in finitediff

@codecov-commenter
Copy link

codecov-commenter commented Mar 1, 2024

Codecov Report

Attention: Patch coverage is 99.92973% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 92.50%. Comparing base (a7cb7dc) to head (d7a9ab7).

Files Patch % Lines
crates/finitediff/src/utils.rs 97.64% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #479      +/-   ##
==========================================
+ Coverage   91.52%   92.50%   +0.97%     
==========================================
  Files         163      178      +15     
  Lines       21731    24577    +2846     
==========================================
+ Hits        19890    22734    +2844     
- Misses       1841     1843       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

let mut xt = x.to_owned();
(0..x.len())
.map(|i| {
let fx1 = mod_and_calc(&mut xt, fs, i, EPS_F64.sqrt());
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For central differencing, you're better off using the cube root so you can improve accuracy to $\epsilon_{\text{machine}}^{2/3}$. With the square root, you'd still only get $\epsilon_{\text{machine}}^{1/2}$ error in the derivative.

Suggested change
let fx1 = mod_and_calc(&mut xt, fs, i, EPS_F64.sqrt());
let fx1 = mod_and_calc(&mut xt, fs, i, EPS_F64.cbrt());

If you choose sqrt, you get the intersection of the red and magenta lines.
image

That said, it's dubious to rely on the problem being carefully nondimensionalized in a library implementaion of finite differencing. The two algorithms I know of are from Walker and Pernice and from Dennis and Schnabel. In my experience, some problems benefit significantly from the more expensive (Dennis and Schnabel) method.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing that out! I followed the Nocedal and Wright book on this (p. 169), and after reading that section again it's clear that I must have missed that part. If I understand it correctly, the book states it should be EPS_F64.cbrt().powi(2), and not EPS_F64.cbrt(). What am I missing?

Thanks for pointing me to the other resources which I wasn't aware of. I will have a look, but probably not as part of this PR.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hah, indeed. That's a mistake on page 169 (it was intended to state that as the achievable error), but it refers to the exercises, which has the correct statement (as in my comment and figure above).

image

In analogy to Equation 7.5, where you want to choose $\epsilon$ to minimize $\epsilon + \symbf u/\epsilon$ (truncation error plus rounding error), when using central differences the truncation error is $O(\epsilon^2)$ so you wish to minimize $\epsilon^2 + \symbf u/\epsilon$, leading to $\epsilon = \symbf u^{1/3}$. (I dropped the $L$ and $L_f$ used to reach (7.6), which scalings like WP or DS seek to respect.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for clarifying and for the explanation! Interestingly, this mistake isn't even mentioned in the errata. I will change it to eps.cbrt() then :)

@stefan-k stefan-k force-pushed the integrate_finitediff branch 3 times, most recently from 4974ea9 to d3661ba Compare March 5, 2024 14:32
@stefan-k stefan-k marked this pull request as ready for review March 5, 2024 14:35
@stefan-k stefan-k merged commit 3f97b77 into argmin-rs:main Mar 5, 2024
31 checks passed
@stefan-k stefan-k deleted the integrate_finitediff branch March 5, 2024 15:58
@stefan-k stefan-k restored the integrate_finitediff branch March 7, 2024 16:15
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

Successfully merging this pull request may close these issues.

3 participants