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

Save response info in storage #6099

Merged
merged 4 commits into from
Sep 26, 2023
Merged

Conversation

oyvindeide
Copy link
Collaborator

Issue
Resolves #my_issue

Approach
Short description of the approach

(Screenshot of new behavior in GUI if applicable)

Pre review checklist

  • Read through the code changes carefully after finishing work
  • Make sure tests pass locally (after every commit!)
  • Prepare changes in small commits for more convenient review (optional)
  • PR title captures the intent of the changes, and is fitting for release notes.
  • Updated documentation
  • Ensured that unit tests are added for all new behavior (See
    Ground Rules),
    and changes to existing code have good test coverage.

Pre merge checklist

  • Added appropriate release note label
  • Commit history is consistent and clean, in line with the contribution guidelines.

@oyvindeide oyvindeide force-pushed the save_response_info branch 9 times, most recently from 05397f6 to 1072844 Compare September 20, 2023 13:39
@codecov-commenter
Copy link

codecov-commenter commented Sep 20, 2023

Codecov Report

Merging #6099 (c1ac285) into main (d5b8101) will increase coverage by 0.20%.
The diff coverage is 97.24%.

@@            Coverage Diff             @@
##             main    #6099      +/-   ##
==========================================
+ Coverage   82.49%   82.70%   +0.20%     
==========================================
  Files         350      351       +1     
  Lines       21455    21540      +85     
  Branches      839      839              
==========================================
+ Hits        17700    17815     +115     
+ Misses       3457     3427      -30     
  Partials      298      298              
Files Coverage Δ
src/ert/callbacks.py 94.82% <100.00%> (ø)
src/ert/cli/main.py 93.58% <ø> (ø)
src/ert/config/__init__.py 100.00% <100.00%> (ø)
src/ert/config/ensemble_config.py 96.06% <100.00%> (+0.06%) ⬆️
src/ert/config/gen_data_config.py 96.87% <100.00%> (-0.05%) ⬇️
src/ert/config/observations.py 93.81% <100.00%> (-0.65%) ⬇️
src/ert/config/parameter_config.py 100.00% <100.00%> (ø)
src/ert/config/response_config.py 93.33% <100.00%> (+4.44%) ⬆️
src/ert/config/summary_config.py 100.00% <100.00%> (ø)
src/ert/enkf_main.py 100.00% <ø> (ø)
... and 13 more

... and 2 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@oyvindeide oyvindeide force-pushed the save_response_info branch 10 times, most recently from 4afec8b to 14978ab Compare September 25, 2023 11:09
Copy link
Contributor

@pinkwah pinkwah left a comment

Choose a reason for hiding this comment

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

Small comments, but looks good!

src/ert/config/gen_data_config.py Outdated Show resolved Hide resolved
src/ert/config/observations.py Show resolved Hide resolved
src/ert/config/summary_config.py Show resolved Hide resolved
src/ert/config/summary_config.py Outdated Show resolved Hide resolved
@oyvindeide oyvindeide force-pushed the save_response_info branch 2 times, most recently from 865a242 to c1ac285 Compare September 26, 2023 07:36
This moves all the information about which responses are saved
in storage to storage, instead of reading them through the config.
The removes the need to pass the ensemble config to the forward
model ok function.
This removes some redundant functions and simplifies
the mocking as some of it was outdated.
@oyvindeide oyvindeide merged commit 4967452 into equinor:main Sep 26, 2023
35 of 39 checks passed
@oyvindeide oyvindeide deleted the save_response_info branch September 26, 2023 10:44
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.

None yet

3 participants