Simplify error estimation by removing _EERepl_ and _delta_ #773

PetroZarytskyi · 2024-02-16T16:06:55Z

This PR simplifies error estimation by removing variables with prefixes _EERepl_ and _delta_. This became possible since we started reusing cloned forward pass variables in the reverse pass. Merging this PR will also unlock #758.
Fixes #757.

codecov · 2024-02-16T16:16:49Z

Codecov Report

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

Project coverage is 94.83%. Comparing base (e930a4a) to head (6d32ee5).

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #773      +/-   ##
==========================================
- Coverage   94.89%   94.83%   -0.07%     
==========================================
  Files          49       49              
  Lines        7478     7333     -145     
==========================================
- Hits         7096     6954     -142     
+ Misses        382      379       -3

Files	Coverage Δ
include/clad/Differentiator/ErrorEstimator.h	`100.00% <100.00%> (ø)`
include/clad/Differentiator/EstimationModel.h	`100.00% <ø> (ø)`
...e/clad/Differentiator/MultiplexExternalRMVSource.h	`100.00% <ø> (ø)`
include/clad/Differentiator/VisitorBase.h	`100.00% <100.00%> (ø)`
lib/Differentiator/ErrorEstimator.cpp	`99.02% <100.00%> (+0.52%)`	⬆️
lib/Differentiator/EstimationModel.cpp	`100.00% <ø> (ø)`
lib/Differentiator/MultiplexExternalRMVSource.cpp	`90.52% <100.00%> (ø)`
lib/Differentiator/ReverseModeVisitor.cpp	`96.59% <100.00%> (+<0.01%)`	⬆️
include/clad/Differentiator/ExternalRMVSource.h	`25.64% <0.00%> (+1.95%)`	⬆️

Files	Coverage Δ
include/clad/Differentiator/ErrorEstimator.h	`100.00% <100.00%> (ø)`
include/clad/Differentiator/EstimationModel.h	`100.00% <ø> (ø)`
...e/clad/Differentiator/MultiplexExternalRMVSource.h	`100.00% <ø> (ø)`
include/clad/Differentiator/VisitorBase.h	`100.00% <100.00%> (ø)`
lib/Differentiator/ErrorEstimator.cpp	`99.02% <100.00%> (+0.52%)`	⬆️
lib/Differentiator/EstimationModel.cpp	`100.00% <ø> (ø)`
lib/Differentiator/MultiplexExternalRMVSource.cpp	`90.52% <100.00%> (ø)`
lib/Differentiator/ReverseModeVisitor.cpp	`96.59% <100.00%> (+<0.01%)`	⬆️
include/clad/Differentiator/ExternalRMVSource.h	`25.64% <0.00%> (+1.95%)`	⬆️

github-actions

clang-tidy made some suggestions

lib/Differentiator/ErrorEstimator.cpp

github-actions · 2024-02-16T16:57:00Z

clang-tidy review says "All clean, LGTM! 👍"

vgvassilev · 2024-02-16T16:59:58Z

@grimmmyshini, could you take a look?

github-actions · 2024-02-16T18:08:04Z

clang-tidy review says "All clean, LGTM! 👍"

github-actions · 2024-02-19T23:19:44Z

clang-tidy review says "All clean, LGTM! 👍"

include/clad/Differentiator/ExternalRMVSource.h

github-actions · 2024-02-20T13:57:40Z

clang-tidy review says "All clean, LGTM! 👍"

github-actions · 2024-02-20T14:12:43Z

clang-tidy review says "All clean, LGTM! 👍"

grimmmyshini · 2024-02-28T10:59:16Z

test/ErrorEstimation/Assignments.C

 //CHECK-NEXT:         * _d_x;
 //CHECK-NEXT:     }
 //CHECK-NEXT:     * _d_y += _d_z;
-//CHECK-NEXT:     _delta_x += std::abs(* _d_x * _EERepl_x0 * {{.+}});


I am making a comment here, but this applies to all the other tests that have been changed similarly.

1.) You cannot get rid of the EERepl vars (at least to my understanding) so easily. In error estimation, we need to "replay" the value of the variable of interest as it was when we started. (Essentially, every variable is a unique instantiation and is never overwritten). For e.g. the following program:

x = x + 1; x = y; y = 20 + x;

Essentially means this to the error estimator:

x_1 = x_0 + 1; x_2 = y_0; y_1 = 20 + x_2;

Then, the final error estimation is essentially this:

final_error = dx_0 * x_0 + dx_1 * x_1 + dx_2 * x_2 + ...

So, you cannot simply replace the eerepl objects with 'x' because most functions will overwrite values multiple times.

2.) You cannot just get rid of the delta vars as you may require them while printing/logging. Sure, we can change this but it makes it significantly easier for us to identify errors through these names in the code.

1.) You cannot get rid of the EERepl vars...

Since 1.3, we started reusing the original variables (x and y) in the reverse sweep. We now restore their values in the reverse sweep. So at every point in the reverse sweep, x has the same value it had in the corresponding point in the forward sweep, not the last value it was equal to (like it was previously). This is why the error estimation statement has been moved to the top of the enclosing reverse-sweep block (because we want the value of x after the assignment). Hopefully, I understand your concern correctly.

2.) You cannot just get rid of the delta vars...

Can't we instead rely on custom error evaluation here (getErrorVal)? I think we can get the same information this way without adding excessive code. Anyway, this seems like something we could discuss in more detail in Slack.

Oh I see! In that case it's fine, you can ignore that comment.

Yeah I mean it's probably okay to get rid of it if you see a worthwhile performance boost. I don't have a strong opinion against it.

grimmmyshini · 2024-02-28T11:06:12Z

I have left a comment in the code addressing one of my major concerns with this PR. That comment applies to every test that has been changed. This PR should also be run on the benchmarks we have for the paper. The resulting error values should also be cross-checked because, as this PR stands, I do not think it is correct and should not be merged.

github-actions · 2024-03-05T15:36:13Z

clang-tidy review says "All clean, LGTM! 👍"

github-actions · 2024-03-05T17:38:54Z

clang-tidy review says "All clean, LGTM! 👍"

PetroZarytskyi · 2024-03-05T21:56:13Z

test/ErrorEstimation/LoopsAndArrays.C

 //CHECK-NEXT:             * _d_x += _d_m * x;
 //CHECK-NEXT:             * _d_x += x * _d_m;
 //CHECK-NEXT:             _d_m = 0;
 //CHECK-NEXT:             m = clad::pop(_t1);
-//CHECK-NEXT:             float _r0 = clad::pop(_EERepl_m0);
-//CHECK-NEXT:             _delta_m += std::abs(_d_m * _r0 * {{.+}});
 //CHECK-NEXT:         }


We should notice that before this PR, we used _d_m after it was set to 0. This is clearly not intended because we should use the old derivative value. This happens every time we declare a variable inside a loop. On the other hand, moving the error statement to the top solves the problem.

PetroZarytskyi · 2024-03-05T22:17:37Z

@grimmmyshini @vgvassilev
This screenshot shows the discrepancy in the benchmark results before (on the right) and after (on the left) this PR:

During our meeting last Sunday, we concluded that the most problematic part is the beta variable.
This is the part of the derivative code that causes this discrepancy:
Before the PR:

{
    double _r57 = clad::pop(_t125);
    double _r58 = _d_beta / _r57;
     _d_rtrans += _r58;
    double _r59 = _d_beta * -clad::pop(_t126) / (_r57 * _r57);
     _d_oldrtrans += _r59;
     _d_beta = 0;              <-- _d_beta is set to 0
     double _r60 = clad::pop(_EERepl_beta0);
     _delta_beta += clad::getErrorVal(_d_beta, _r60, "beta");    <-- the error is 0
}

After the PR:

{
    _final_error += clad::getErrorVal(_d_beta, beta, "beta");  <-- the old value of _d_beta is used
    _d_rtrans += _d_beta / oldrtrans;
    double _r1 = _d_beta * -rtrans / (oldrtrans * oldrtrans);
    _d_oldrtrans += _r1;
    _d_beta = 0;
    beta = clad::pop(_t48);
}

From what I understand, the new behavior is correct here. You can also see my comment above describing the same in one of the tests. Luckily, this doesn't affect the conclusions drawn in the article.

github-actions · 2024-03-06T17:08:48Z

clang-tidy review says "All clean, LGTM! 👍"

vgvassilev · 2024-03-10T07:46:32Z

@grimmmyshini, ping.

grimmmyshini · 2024-03-10T13:57:24Z

I see! Well, it's weird that we missed it in the first go but no issue. If you have verified all the error results from the tests, then this PR is ready to go. 👍

github-actions · 2024-03-10T15:23:20Z

clang-tidy review says "All clean, LGTM! 👍"

vgvassilev · 2024-03-10T15:24:23Z

@PetroZarytskyi, any chances to increase the testing coverage of this patch?

PetroZarytskyi · 2024-03-12T16:32:23Z

@vgvassilev

@PetroZarytskyi, any chances to increase the testing coverage of this patch?

I looked into the CodeDev report. At first, I was confused how it was possible that the coverage of every single test either increased or didn't change. Then I realized that I made ErrorEstimator.cpp way smaller and that decreased the average project coverage. This happened because the coverage of ErrorEstimator.cpp is higher than average. So I think making the coverage higher again would involve doing that for lines unrelated to the PR.

vgvassilev · 2024-03-12T17:18:55Z

@vgvassilev

@PetroZarytskyi, any chances to increase the testing coverage of this patch?

I looked into the CodeDev report. At first, I was confused how it was possible that the coverage of every single test either increased or didn't change. Then I realized that I made ErrorEstimator.cpp way smaller and that decreased the average project coverage. This happened because the coverage of ErrorEstimator.cpp is higher than average. So I think making the coverage higher again would involve doing that for lines unrelated to the PR.

That was my take as well. Thanks for the explanation. Let's move forward.

vgvassilev

Well done! Thank you! We can ignore the codecov as we reduced the code significantly and that's what makes codecov unhappy but makes everybody else happy...

PetroZarytskyi force-pushed the remove-eerepl branch from edda5ac to 22be703 Compare February 16, 2024 16:09

github-actions bot reviewed Feb 16, 2024

View reviewed changes

PetroZarytskyi force-pushed the remove-eerepl branch 2 times, most recently from 639fafb to 80052c5 Compare February 16, 2024 16:45

vgvassilev requested a review from grimmmyshini February 16, 2024 16:59

PetroZarytskyi force-pushed the remove-eerepl branch 2 times, most recently from e1b3715 to dd15512 Compare February 16, 2024 17:55

vgvassilev reviewed Feb 20, 2024

View reviewed changes

include/clad/Differentiator/ExternalRMVSource.h Outdated Show resolved Hide resolved

grimmmyshini reviewed Feb 28, 2024

View reviewed changes

PetroZarytskyi force-pushed the remove-eerepl branch from 4673c5d to 950512b Compare March 5, 2024 15:21

PetroZarytskyi commented Mar 5, 2024

View reviewed changes

vgvassilev force-pushed the remove-eerepl branch from 760e3c2 to 66e00c2 Compare March 10, 2024 14:52

PetroZarytskyi added 4 commits March 10, 2024 15:06

Simplify error estimation by removing _EERepl_ and _delta_

f25dde9

Improve test coverage by removing dead code

b1bdc4e

Add a test for error estimation for integer LHS in assignment

46ab87f

Rename Finalising to Finalizing

dc26eed

Don't emit error statements when assigning array sub exprs

6d32ee5

vgvassilev force-pushed the remove-eerepl branch from 66e00c2 to 6d32ee5 Compare March 10, 2024 15:07

vgvassilev approved these changes Mar 12, 2024

View reviewed changes

vgvassilev merged commit 55835f0 into vgvassilev:master Mar 12, 2024
81 of 83 checks passed

PetroZarytskyi deleted the remove-eerepl branch March 12, 2024 21:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify error estimation by removing _EERepl_ and _delta_ #773

Simplify error estimation by removing _EERepl_ and _delta_ #773

PetroZarytskyi commented Feb 16, 2024 •

edited

Loading

codecov bot commented Feb 16, 2024 •

edited

Loading

github-actions bot left a comment

github-actions bot commented Feb 16, 2024

vgvassilev commented Feb 16, 2024

github-actions bot commented Feb 16, 2024

github-actions bot commented Feb 19, 2024

github-actions bot commented Feb 20, 2024

github-actions bot commented Feb 20, 2024

grimmmyshini Feb 28, 2024

PetroZarytskyi Feb 28, 2024

PetroZarytskyi Feb 28, 2024

grimmmyshini Feb 28, 2024

grimmmyshini commented Feb 28, 2024 •

edited

Loading

github-actions bot commented Mar 5, 2024

github-actions bot commented Mar 5, 2024

PetroZarytskyi Mar 5, 2024 •

edited

Loading

PetroZarytskyi commented Mar 5, 2024 •

edited by vgvassilev

Loading

github-actions bot commented Mar 6, 2024

vgvassilev commented Mar 10, 2024

grimmmyshini commented Mar 10, 2024

github-actions bot commented Mar 10, 2024

vgvassilev commented Mar 10, 2024

PetroZarytskyi commented Mar 12, 2024

vgvassilev commented Mar 12, 2024

vgvassilev left a comment

Simplify error estimation by removing _EERepl_ and _delta_ #773

Simplify error estimation by removing _EERepl_ and _delta_ #773

Conversation

PetroZarytskyi commented Feb 16, 2024 • edited Loading

codecov bot commented Feb 16, 2024 • edited Loading

Codecov Report

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot commented Feb 16, 2024

vgvassilev commented Feb 16, 2024

github-actions bot commented Feb 16, 2024

github-actions bot commented Feb 19, 2024

github-actions bot commented Feb 20, 2024

github-actions bot commented Feb 20, 2024

grimmmyshini Feb 28, 2024

Choose a reason for hiding this comment

PetroZarytskyi Feb 28, 2024

Choose a reason for hiding this comment

PetroZarytskyi Feb 28, 2024

Choose a reason for hiding this comment

grimmmyshini Feb 28, 2024

Choose a reason for hiding this comment

grimmmyshini commented Feb 28, 2024 • edited Loading

github-actions bot commented Mar 5, 2024

github-actions bot commented Mar 5, 2024

PetroZarytskyi Mar 5, 2024 • edited Loading

Choose a reason for hiding this comment

PetroZarytskyi commented Mar 5, 2024 • edited by vgvassilev Loading

github-actions bot commented Mar 6, 2024

vgvassilev commented Mar 10, 2024

grimmmyshini commented Mar 10, 2024

github-actions bot commented Mar 10, 2024

vgvassilev commented Mar 10, 2024

PetroZarytskyi commented Mar 12, 2024

vgvassilev commented Mar 12, 2024

vgvassilev left a comment

Choose a reason for hiding this comment

PetroZarytskyi commented Feb 16, 2024 •

edited

Loading

codecov bot commented Feb 16, 2024 •

edited

Loading

grimmmyshini commented Feb 28, 2024 •

edited

Loading

PetroZarytskyi Mar 5, 2024 •

edited

Loading

PetroZarytskyi commented Mar 5, 2024 •

edited by vgvassilev

Loading