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

Fix possible error in jacobian tests #358

Merged
merged 1 commit into from
Jan 16, 2025
Merged

Conversation

ronald-jaepel
Copy link
Collaborator

@ronald-jaepel ronald-jaepel commented Jan 15, 2025

During the jacobian tests we compare the residual calculated with finite differences in both directions (stored in colA and colB):

... if (std::isnan(colA[row]))
  CHECK(std::isnan(colB[row]));
else
  CHECK(std::signbit(colA[row]) == std::signbit(colB[row]));

However, the first test is:

if (std::abs(colA[row]) <= absTol)
  CHECK(std::abs(colA[row]) <= absTol);

where we compare colA to colA. To me this seems like a typo, because very other check compares the result in colA with colB.

With this change some test fail now.

ToDos:

  • someone else confirm that this change is reasonable
  • fix tests again
    • GRM_DG multiple particle types Jacobian analytic vs AD
    • GRM_DG dynamic reactions Jacobian vs AD bulk
    • GRM_DG dynamic reactions Jacobian vs AD particle
    • GRM_DG dynamic reactions Jacobian vs AD modified particle
    • GRM_DG dynamic reactions Jacobian vs AD bulk and particle
    • GRM_DG dynamic reactions Jacobian vs AD bulk and modified particle
    • Crystallization Jacobian verification for a DPFR/LRM with primary and secondary nucleation and growth

@jbreue16 jbreue16 force-pushed the fix/jacobian_test_comparison branch from 2ef7aaa to 17698ee Compare January 16, 2025 10:01
Copy link
Contributor

@jbreue16 jbreue16 left a comment

Choose a reason for hiding this comment

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

Thanks for catching this !
Turns out we just need to adapt tolerances within a reasonable range :)

@jbreue16 jbreue16 merged commit e054d22 into master Jan 16, 2025
4 checks passed
@jbreue16 jbreue16 deleted the fix/jacobian_test_comparison branch January 16, 2025 10:14
@github-actions github-actions bot locked and limited conversation to collaborators Jan 16, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants