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 sunsch updated time_vector.py #722

Merged
merged 1 commit into from
Aug 16, 2024
Merged

Conversation

alifbe
Copy link
Collaborator

@alifbe alifbe commented Aug 16, 2024

Use @vkip updated time_vector.py to be compatible with opm 2024.4

OPM/opm-common#4165

@alifbe alifbe marked this pull request as ready for review August 16, 2024 10:06
@alifbe alifbe requested review from a team and asnyv August 16, 2024 10:06
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 89.61039% with 16 lines in your changes missing coverage. Please review.

Project coverage is 85.73%. Comparing base (606c558) to head (1653e62).

Files Patch % Lines
src/subscript/sunsch/time_vector.py 89.54% 16 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #722      +/-   ##
==========================================
+ Coverage   85.65%   85.73%   +0.08%     
==========================================
  Files          49       50       +1     
  Lines        6544     6697     +153     
==========================================
+ Hits         5605     5742     +137     
- Misses        939      955      +16     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@asnyv
Copy link
Contributor

asnyv commented Aug 16, 2024

We just removed the copied code from last time there was a fix in opm (#720). I really think we should try to avoid having this code in subscript. Either push for a patch release in opm-common or wait for the next (as far as I know we have lived without the functionality in OPM/opm-common#4165 for a long time.

@alifbe
Copy link
Collaborator Author

alifbe commented Aug 16, 2024

We just removed the copied code from last time there was a fix in opm (#720). I really think we should try to avoid having this code in subscript. Either push for a patch release in opm-common or wait for the next (as far as I know we have lived without the functionality in OPM/opm-common#4165 for a long time.

@asnyv Yes, I totally agreed with you. But we got a report where user has crash when they have BRANPROP keyword in schedule section due to the changes in OPM 2024.4
https://equinor.slack.com/archives/CKC3ST0UT/p1723631464314409

We will revert this once PR in OPM is released (probably in 2024.10)

@asnyv
Copy link
Contributor

asnyv commented Aug 16, 2024

@alifbe alternatively pin opm to 2023.10 until the fix is in place in 2024.10?

@mferrera
Copy link
Collaborator

Seeking to pin opm for the August Komodo was part of the plan as well. In our discussion about this, and trying to account for all the moving parts with RHEL8 etc, we thought reverting this change would be needed too. But I think now, maybe it is not?

@asnyv
Copy link
Contributor

asnyv commented Aug 16, 2024

I'm not going to completely block this PR of course, but I would at least recommend to check if a pin is sufficient first.

@alifbe alifbe merged commit c7dde90 into equinor:main Aug 16, 2024
6 checks passed
@alifbe alifbe deleted the use-sunsch-timevector branch August 16, 2024 12:13
@mferrera
Copy link
Collaborator

For posterity, we decided to merge this as it contains an additional patch fix going into OPM's time_vector.py as well, and pinning OPM < 2024.4 interferes will cause us to lack RHEL8 compatibility. We will revert the change when 2024.10 comes out

@alifbe alifbe mentioned this pull request Aug 19, 2024
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