Skip to content

Script for CO2 containment#568

Merged
mferrera merged 11 commits intoequinor:mainfrom
AudunSektnanNR:co2_containment_final
Jun 30, 2023
Merged

Script for CO2 containment#568
mferrera merged 11 commits intoequinor:mainfrom
AudunSektnanNR:co2_containment_final

Conversation

@AudunSektnanNR
Copy link
Copy Markdown
Contributor

Script for calculating the amount of CO2 inside and outside a given perimeter, and separates the result per formation and phase (gas/dissolved). Output is a table on CSV format.

Can take both a containment polygon and a "hazardous" polygon. Result is divided into the regions inside vs outside vs hazardous, where inside means inside the containment polygon.

Can calculate either CO2 mass or a representative CO2 volume (three different calculation methods for volume). CO2 mass is the most used and tested.

The resulting CSV-file can be used as data in the CO2Leakage webviz plugin.

See (old) issue: #502

…perimeter, and separates the result per formation and phase (gas/dissolved). Output is a table on CSV format.
@AudunSektnanNR AudunSektnanNR marked this pull request as ready for review June 27, 2023 13:33
Copy link
Copy Markdown
Collaborator

@alifbe alifbe left a comment

Choose a reason for hiding this comment

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

Please fix the order of import (using isort or vscode)

Comment thread src/subscript/co2containment/calculate.py Outdated
Comment thread src/subscript/co2containment/co2_calculation.py
@AudunSektnanNR
Copy link
Copy Markdown
Contributor Author

Have fixed some of the PR issues. Still some error from mypy-checks. Looking into that.

Comment thread docs/scripts/co2_containment.rst Outdated
Comment thread src/subscript/co2containment/calculate.py
Comment thread src/subscript/co2containment/co2_containment.py Outdated
Comment thread tests/test_co2_calculate.py Outdated
Comment thread tests/test_co2_containment.py Outdated
Comment thread tests/test_co2_containment.py Outdated
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jun 28, 2023

Codecov Report

Merging #568 (d601a66) into main (68957a2) will decrease coverage by 1.46%.
The diff coverage is 62.36%.

❗ Current head d601a66 differs from pull request most recent head 691a0ca. Consider uploading reports for the commit 691a0ca to get more accurate results

@@            Coverage Diff             @@
##             main     #568      +/-   ##
==========================================
- Coverage   86.16%   84.71%   -1.46%     
==========================================
  Files          49       52       +3     
  Lines        7035     7492     +457     
==========================================
+ Hits         6062     6347     +285     
- Misses        973     1145     +172     
Impacted Files Coverage Δ
src/subscript/co2_containment/co2_containment.py 44.06% <44.06%> (ø)
src/subscript/co2_containment/co2_calculation.py 67.10% <67.10%> (ø)
src/subscript/co2_containment/calculate.py 82.85% <82.85%> (ø)

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

@AudunSektnanNR AudunSektnanNR requested a review from alifbe June 28, 2023 11:48
@mferrera
Copy link
Copy Markdown
Collaborator

Looking good! 😄 Thanks a lot for adapting to the linters, a bit of a gauntlet to be sure.

My last concern is with the test coverage -- it becomes our third outlier at significantly below 80% coverage and more importantly we lack the CO2 domain knowledge that may prevent us from addressing issues ourselves. I guess normally coverage is the sort of thing that increases over time as users do unexpected user things, but because we are doing a change of hands it is a bit more of a worry.

Being aware that you have a time runway to work with, do you think there are any critical or highly domain specific methods that could potentially add to long-term robustness?

@AudunSektnanNR AudunSektnanNR requested a review from mferrera June 28, 2023 11:48
@AudunSektnanNR
Copy link
Copy Markdown
Contributor Author

When it comes to test coverage, I think the best thing would be to have a synthetic data set (EGRID, UNRST, INIT files) with all the required properties used for the co2 calculations. Then it would be easier to test a larger part of the code. But this is probably a bit of work to get done, so realistically this will have to be added sometime in the future I think.

I added a few more tests, so the code coverage will hopefully increase slightly.

Copy link
Copy Markdown
Collaborator

@mferrera mferrera left a comment

Choose a reason for hiding this comment

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

Thanks very much for considering the comments!

We will need to have a quick consensus chat on our end before taking it in so it might be a short while longer -- early next week at the very latest.

I will add co2_containment as the script invocation unless you have some other preferred command line name (doesn't make sense to add it in this PR since it will be moved in #557)

@AudunSektnanNR
Copy link
Copy Markdown
Contributor Author

Sounds good! Thank you for all the feedback and fast responses, I think it improved the code.

The proposed script invocation is fine!

Copy link
Copy Markdown
Collaborator

@alifbe alifbe left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@mferrera mferrera merged commit 6b443b8 into equinor:main Jun 30, 2023
@AudunSektnanNR AudunSektnanNR deleted the co2_containment_final branch August 25, 2023 07:34
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.

4 participants