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

Use ies #2602

Merged
merged 1 commit into from
Jan 19, 2022
Merged

Use ies #2602

merged 1 commit into from
Jan 19, 2022

Conversation

joakim-hove
Copy link
Contributor

@joakim-hove joakim-hove commented Dec 15, 2021

Approach
On top of: #2412

With this PR the ies_enkf_initX() implementation is used also for the std_enkf module.

Pre review checklist

  • Added appropriate labels

@joakim-hove joakim-hove added analysis maintenance Not a bug now but could be one day, repaying technical debt labels Dec 15, 2021
@joakim-hove joakim-hove mentioned this pull request Dec 20, 2021
@joakim-hove
Copy link
Contributor Author

This is ready for review.

With this PR the std_enkf module uses the same internal implementation of analysis update as the ies module - i.e. although there are still superficially two modules remaining, there is only one underlying implementation.

@codecov-commenter
Copy link

codecov-commenter commented Jan 17, 2022

Codecov Report

Merging #2602 (49443d4) into main (de59fc1) will decrease coverage by 0.08%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2602      +/-   ##
==========================================
- Coverage   65.33%   65.25%   -0.09%     
==========================================
  Files         652      652              
  Lines       53660    53654       -6     
  Branches     4733     4774      +41     
==========================================
- Hits        35060    35010      -50     
- Misses      17067    17086      +19     
- Partials     1533     1558      +25     
Impacted Files Coverage Δ
libres/lib/analysis/ies/ies.cpp 56.36% <100.00%> (+0.11%) ⬆️
libres/lib/analysis/std_enkf.cpp 57.85% <100.00%> (-1.79%) ⬇️
libres/lib/analysis/update.cpp 34.37% <0.00%> (-8.97%) ⬇️
libres/lib/analysis/enkf_linalg.cpp 40.07% <0.00%> (-6.31%) ⬇️
libres/lib/res_util/matrix.cpp 65.40% <0.00%> (-0.82%) ⬇️
libres/lib/enkf/block_fs_driver.cpp 81.35% <0.00%> (-0.57%) ⬇️
res/fm/templating/template_render.py 84.00% <0.00%> (-0.32%) ⬇️
res/enkf/__init__.py 100.00% <0.00%> (ø)
libres/lib/python/init.cpp 0.00% <0.00%> (ø)
... and 11 more

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 de59fc1...49443d4. Read the comment docs.

@@ -39,8 +39,7 @@ void init_update(void *arg, const bool_vector_type *ens_mask,
const matrix_type *R, const matrix_type *dObs,
const matrix_type *E, const matrix_type *D, rng_type *rng);

void initX(const std::variant<double, int> &truncation,
config::inversion_type ies_inversion, const matrix_type *Y0,
void initX(const config::config_type *ies_config, const matrix_type *S,
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think of adding a docstring to this in Doxygen format, something like:

/**
 * @brief 
 * 
 * @param ies_config 
 * @param S 
 * @param R 
 * @param E 
 * @param D 
 * @param X 
 */

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Personally I think Doxygen means "old and irrelevant" - so I would prefer not to.

Copy link
Contributor

@dafeda dafeda Jan 18, 2022

Choose a reason for hiding this comment

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

Which format of docstrings do you prefer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My general attitude is that I am skeptical to documentation strings, because I feel they very often are stale and irrelevant[1]. The formatted strings (typically doxygen) even more so, because I suspect the guy inserting that comment used a shortcut in his editor with thinking much about it - i.e. it screams: "Comment without love".

My preferred way to handle comments is that when something is really complex/broken/.... and I see no obvious way to improve the problem away I write a comment. The comment can be quite extensive, it is in free form and typically for developers actually working with the problematic code - i.e. in the .cpp file. In addition to the text in the comment the mere fact that I have actually added a comment should signal "here be dragons" for the pour soul coming after me.

But I am pushing 50 and have no spine left - will do as encouraged.

[1]: Due to pydoc I do feel quite different about this in Python - here the comments/document strings are available to a much larger audience and it feels meaningful to write them.

Copy link
Contributor

Choose a reason for hiding this comment

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

My general attitude is that I am skeptical to documentation strings, because I feel they very often are stale and irrelevant[1]. The formatted strings (typically doxygen) even more so, because I suspect the guy inserting that comment used a shortcut in his editor with thinking much about it - i.e. it screams: "Comment without love".

The staleness is in my opinion due to docstrings containing too much detail.
In this specific case I would write that initX is equivalent to a standard EnKF implementation but that it uses an iterative ensemble smoother under the hood or something like that.

My preferred way to handle comments is that when something is really complex/broken/.... and I see no obvious way to improve the problem away I write a comment. The comment can be quite extensive, it is in free form and typically for developers actually working with the problematic code - i.e. in the .cpp file. In addition to the text in the comment the mere fact that I have actually added a comment should signal "here be dragons" for the pour soul coming after me.

I think this is fine, but should ideally be dealt with fast.
In other words, such comments should not grow old, which they often do.

But I am pushing 50 and have no spine left - will do as encouraged.

The spine hardens with age!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added a comment which I am happy with.

@joakim-hove
Copy link
Contributor Author

joakim-hove commented Jan 18, 2022

In this specific case I would write that initX is equivalent to a standard EnKF implementation but that it uses an iterative ensemble smoother under the hood or something like that.

Well - that is actually wrong. It uses the standard EnKF implementation which is an internal part of the iterated algorithm. But that aside: I think your comment illustrates an interesting point, when writing comments we feel inclined to document the journey we have completed - that is not really interesting to someone coming after us. When this PR is merged the initX() routine will be the enkf routine.

@dafeda
Copy link
Contributor

dafeda commented Jan 18, 2022

In this specific case I would write that initX is equivalent to a standard EnKF implementation but that it uses an iterative ensemble smoother under the hood or something like that.

Well - that is actually wrong.
It uses the standard EnKF implementation which is an internal part of the iterated algorithm. But that aside: I think your comment illustrates an interesting point, when writing comments we feel inclined to document the journey we have completed - that is not really interesting to someone coming after us.

I thought that the iterated algorithm was for the case when the posterior is not Gaussian and when predictions and state vectors were non-linearly related.
That is, I thought the iterated algorithm made fewer assumptions than EnKF.
In any case, the journey completed provides context which is of great importance to those coming after us IMO.

@joakim-hove
Copy link
Contributor Author

I thought that the iterated algorithm was for the case when the posterior is not Gaussian and when predictions and state vectors were non-linearly related.
That is, I thought the iterated algorithm made fewer assumptions than EnKF.

Both the EnKF and iterated are for the nonlinear case, however the iterated makes smaller steps - i.e. less severe effects of nonlinearity.

In any case, the journey completed provides context which is of great importance to those coming after us IMO.

Here we disagree - or at least I think this will lead to very much comments.

@joakim-hove
Copy link
Contributor Author

Have updated the comment in the .hpp slightly and added a more verbose comment in the .cpp file

Copy link
Contributor

@dafeda dafeda left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@joakim-hove joakim-hove enabled auto-merge (rebase) January 19, 2022 07:24
@joakim-hove joakim-hove merged commit d71b9fe into equinor:main Jan 19, 2022
@joakim-hove joakim-hove deleted the use-ies branch January 24, 2022 11:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Not a bug now but could be one day, repaying technical debt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants