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

Remove analysis enums #3283

Merged
merged 3 commits into from
Apr 26, 2022
Merged

Remove analysis enums #3283

merged 3 commits into from
Apr 26, 2022

Conversation

ManInFez
Copy link
Contributor

Issue
Resolves #3266

Approach
Short description of the approach

Pre review checklist

  • Added appropriate release note label
  • PR title captures the intent of the changes, and is fitting for release notes.
  • Commit history is consistent and clean, in line with the contribution guidelines.

Adding labels helps the maintainers when writing release notes. This is the list of release note labels.

@ManInFez ManInFez added the release-notes:breaking-change Automatically categorise as breaking change in release notes label Apr 21, 2022
@ManInFez ManInFez self-assigned this Apr 21, 2022
@ManInFez ManInFez force-pushed the remove_analysis_enums branch 2 times, most recently from cc76208 to 19281ed Compare April 21, 2022 12:47
@ManInFez
Copy link
Contributor Author

test this please!

res/enkf/es_update.py Outdated Show resolved Hide resolved
ert_gui/simulation/ensemble_smoother_panel.py Outdated Show resolved Hide resolved
@ManInFez ManInFez force-pushed the remove_analysis_enums branch 5 times, most recently from 878c3e1 to 4e8d3fb Compare April 25, 2022 07:25
@ManInFez
Copy link
Contributor Author

test this please!

elif variable_type == int:
return analysis_module.getInt(name)
else:
logger.error(f"Unknown variable: {name} of type: {variable_type}")
Copy link
Collaborator

Choose a reason for hiding this comment

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

The replacement here no longer has an: else, should perhaps add that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Asserting variable in getvar-function and raising exception if not valid nor supported type

Copy link
Collaborator

@oyvindeide oyvindeide left a comment

Choose a reason for hiding this comment

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

Small comment, other than that LGTM! Good job cleaning!

The analysis module is strictly connected to which type of experiment
is being ran. ES and ES_MDA use STD_ENKF and IES uses IES_ENKF. These
are now hard coded
@ManInFez ManInFez force-pushed the remove_analysis_enums branch 3 times, most recently from 1aa5890 to 659bdb1 Compare April 25, 2022 13:37
@codecov-commenter
Copy link

codecov-commenter commented Apr 25, 2022

Codecov Report

Merging #3283 (0aa20ae) into main (e8df05a) will decrease coverage by 0.08%.
The diff coverage is 84.12%.

❗ Current head 0aa20ae differs from pull request most recent head 4656aef. Consider uploading reports for the commit 4656aef to get more accurate results

@@            Coverage Diff             @@
##             main    #3283      +/-   ##
==========================================
- Coverage   64.96%   64.87%   -0.09%     
==========================================
  Files         618      615       -3     
  Lines       48498    48356     -142     
  Branches     4363     4359       -4     
==========================================
- Hits        31506    31373     -133     
+ Misses      15543    15528      -15     
- Partials     1449     1455       +6     
Impacted Files Coverage Δ
ert_gui/ertwidgets/analysismodulevariablespanel.py 20.58% <ø> (ø)
ert_shared/cli/main.py 86.95% <ø> (ø)
ert_shared/libres_facade.py 93.03% <ø> (-0.53%) ⬇️
libres/lib/analysis/analysis_module.cpp 28.24% <ø> (-0.81%) ⬇️
libres/lib/enkf/analysis_config.cpp 56.56% <0.00%> (+0.75%) ⬆️
.../ertwidgets/models/analysismodulevariablesmodel.py 78.78% <25.00%> (+21.01%) ⬆️
ert_gui/tools/run_analysis/run_analysis_panel.py 36.36% <50.00%> (-1.14%) ⬇️
res/analysis/analysis_module.py 85.52% <71.42%> (-1.98%) ⬇️
ert_gui/ertwidgets/analysismoduleedit.py 88.88% <88.88%> (ø)
ert_gui/ertwidgets/__init__.py 75.00% <100.00%> (ø)
... and 59 more

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@ManInFez ManInFez merged commit 94901f6 into equinor:main Apr 26, 2022
@ManInFez ManInFez deleted the remove_analysis_enums branch April 26, 2022 08:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes:breaking-change Automatically categorise as breaking change in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove unused LIB_IES
3 participants