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

Test obs_data_allocE #2530

Merged
merged 1 commit into from
Dec 11, 2021
Merged

Test obs_data_allocE #2530

merged 1 commit into from
Dec 11, 2021

Conversation

dafeda
Copy link
Contributor

@dafeda dafeda commented Dec 8, 2021

Issue
Resolves #2529

Approach
Short description of the approach

@dafeda dafeda self-assigned this Dec 8, 2021
@@ -274,8 +273,8 @@ static void obs_block_initR(const obs_block_type *obs_block, matrix_type *R,
matrix_free(obs_block->error_covar);
}

static void obs_block_initE(const obs_block_type *obs_block, matrix_type *E,
const double *pert_var, int *__obs_offset) {
void obs_block_initE(const obs_block_type *obs_block, matrix_type *E,
Copy link
Contributor

Choose a reason for hiding this comment

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

The details of the obs_block was originally meant to be an internal implementation detail of obs_data - I would recommend doing the testing fully through the obs_data api.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean that I should not test the obs_block_initE function directly, but instead just test obs_data_allocE?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would do that yes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense, thanks!
I've pushed a fix.

@dafeda dafeda force-pushed the test_obs_data_allocE branch 2 times, most recently from adcbef2 to 3d38575 Compare December 9, 2021 07:33
@codecov-commenter
Copy link

codecov-commenter commented Dec 9, 2021

Codecov Report

Merging #2530 (7d2510d) into main (6e14454) will increase coverage by 0.04%.
The diff coverage is n/a.

❗ Current head 7d2510d differs from pull request most recent head 0c71514. Consider uploading reports for the commit 0c71514 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2530      +/-   ##
==========================================
+ Coverage   64.33%   64.37%   +0.04%     
==========================================
  Files         645      645              
  Lines       54318    54323       +5     
  Branches     4538     4538              
==========================================
+ Hits        34947    34973      +26     
+ Misses      18001    17967      -34     
- Partials     1370     1383      +13     
Impacted Files Coverage Δ
libres/lib/enkf/obs_data.cpp 47.50% <ø> (+12.89%) ⬆️
ert_gui/ertwidgets/validationsupport.py 79.45% <0.00%> (-19.18%) ⬇️
ert_gui/ertwidgets/__init__.py 75.00% <0.00%> (-2.28%) ⬇️
libres/lib/enkf/block_fs_driver.cpp 81.35% <0.00%> (-0.57%) ⬇️
libres/lib/res_util/block_fs.cpp 53.22% <0.00%> (-0.15%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6e14454...0c71514. Read the comment docs.

@dafeda dafeda force-pushed the test_obs_data_allocE branch 3 times, most recently from 3679519 to 56d2690 Compare December 9, 2021 15:20
@BjarneHerland
Copy link
Contributor

An observation: The differences btw linux and mac in this example is surprisingly symmetric. Could it be caused by some indexing-issue instead of actual difference in sequences?

@dafeda
Copy link
Contributor Author

dafeda commented Dec 10, 2021

An observation: The differences btw linux and mac in this example is surprisingly symmetric. Could it be caused by some indexing-issue instead of actual difference in sequences?

Looks like it's because of the way the elements of the matrix E are calculated.

Each element of E is multiplied by a factor.
The factor is a function of the elements of E, which means that it changes together with E and "cancels" out some of the differences in std::normal_distribution.
Why some numbers change sign is I think just due to chance, but I have not looked that deeply into it.

Does that make sense?

Here are the admittedly poorly formatted numbers that go into the calculation of E:

Linux:

E[i, j]: -0.937757 factor: 0.319912 E[i, j] * factor: -0.3
E[i, j]: 0.937757 factor: 0.319912 E[i, j] * factor: 0.3
E[i, j]: -0.959462 factor: 0.521125 E[i, j] * factor: -0.5
E[i, j]: 0.959462 factor: 0.521125 E[i, j] * factor: 0.5
E[i, j]: 0.369076 factor: 0.812841 E[i, j] * factor: 0.3
E[i, j]: -0.369076 factor: 0.812841 E[i, j] * factor: -0.3
E[i, j]: -0.11267 factor: 4.43772 E[i, j] * factor: -0.5
E[i, j]: 0.11267 factor: 4.43772 E[i, j] * factor: 0.5
E[i, j]: -0.204426 factor: 1.46752 E[i, j] * factor: -0.3
E[i, j]: 0.204426 factor: 1.46752 E[i, j] * factor: 0.3
E[i, j]: -0.239391 factor: 2.08863 E[i, j] * factor: -0.5
E[i, j]: 0.239391 factor: 2.08863 E[i, j] * factor: 0.5
E[i, j]: -1.17627 factor: 0.510086 E[i, j] * factor: -0.6
E[i, j]: 1.17627 factor: 0.510086 E[i, j] * factor: 0.6

MacOS:

E[i, j]: 0.914876 factor: 0.327913 E[i, j] * factor: 0.3
E[i, j]: -0.914876 factor: 0.327913 E[i, j] * factor: -0.3
E[i, j]: -0.264962 factor: 1.88706 E[i, j] * factor: -0.5
E[i, j]: 0.264962 factor: 1.88706 E[i, j] * factor: 0.5
E[i, j]: 0.150479 factor: 1.99363 E[i, j] * factor: 0.3
E[i, j]: -0.150479 factor: 1.99363 E[i, j] * factor: -0.3
E[i, j]: -0.0873137 factor: 5.72648 E[i, j] * factor: -0.5
E[i, j]: 0.0873137 factor: 5.72648 E[i, j] * factor: 0.5
E[i, j]: -0.842567 factor: 0.356055 E[i, j] * factor: -0.3
E[i, j]: 0.842567 factor: 0.356055 E[i, j] * factor: 0.3
E[i, j]: 0.295955 factor: 1.68945 E[i, j] * factor: 0.5
E[i, j]: -0.295955 factor: 1.68945 E[i, j] * factor: -0.5
E[i, j]: 0.346664 factor: 1.73078 E[i, j] * factor: 0.6
E[i, j]: -0.346664 factor: 1.73078 E[i, j] * factor: -0.6

@dafeda dafeda force-pushed the test_obs_data_allocE branch 2 times, most recently from 0c71514 to 2338873 Compare December 10, 2021 12:54
@dafeda
Copy link
Contributor Author

dafeda commented Dec 10, 2021

The "symmetry" is a consequence of centering the matrix with only two realizations.

Here's the first part of the calculations done in obs_data_allocE:

nens = 2 # Size of ensemble
nobs = 5 # Number of observations

E = rng.normal(0, 1, size=(nobs, nens))
E - E.mean(axis=1).reshape(nobs, 1)

This results in:

array([[-0.14346748, 0.14346748],
[-0.50043274, 0.50043274],
[ 0.2308551 , -0.2308551 ],
[-0.63106343, 0.63106343],
[-0.9199802 , 0.9199802 ]])

This "symmetry" goes away when using more than two realizations, so I've updated my tests.

@dafeda
Copy link
Contributor Author

dafeda commented Dec 10, 2021

For completeness, here's the rest of the calculation that produces the same results as were used in the test before increasing the number of realizations:

I will put this in the documentation of the test.

E = rng.normal(0, 1, size=(5, 2))
nens = E.shape[1]
nobs = E.shape[0]

E_centered = E - E.mean(axis=1).reshape(nobs, 1)

# Estimate of variance
pert_var = (E_centered * E_centered).sum(axis=1)

obs_block_std = np.array([0.3, 0.5, 0.3, 0.5, 0.6])

factor = obs_block_std.reshape(nobs, 1) * np.sqrt(
    nens / pert_var.reshape(nobs, 1)
)

E_final = E_centered * factor

E_final

array([[ 0.3, -0.3],
[-0.5, 0.5],
[ 0.3, -0.3],
[ 0.5, -0.5],
[ 0.6, -0.6]])

Copy link
Contributor

@BjarneHerland BjarneHerland left a comment

Choose a reason for hiding this comment

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

Apart for the nits, this LGTM.

@dafeda dafeda merged commit ee2ac23 into equinor:main Dec 11, 2021
@dafeda dafeda deleted the test_obs_data_allocE branch December 11, 2021 12:43
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.

Test obs_data_allocE
4 participants