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

Update wsegaicd for future regression testing. #853

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

tklov11
Copy link
Contributor

@tklov11 tklov11 commented Nov 10, 2022

This PR prepares the opm-tests/wsegaicd case for future regression testing when new segment and connection vectors in the RFT-file are implemented.

@alfbr
Copy link
Member

alfbr commented Nov 10, 2022

Thanks for extending the test coverage, this is a welcome improvement. Are you planning further changes or do you want this merged as is?

@tklov11
Copy link
Contributor Author

tklov11 commented Nov 10, 2022

I do not think there is any rush to merge this yet. There is a review process in the offing on the new additions to WRFTPLT-data that will take a week or two before available in master/main. I will talk with @tskille on the details for the regression testing, which I am not very familiar with.

@bska
Copy link
Member

bska commented Nov 28, 2022

jenkins build this please

@bska
Copy link
Member

bska commented Nov 30, 2022

I was a little confused that the build check succeeded, but now I see that we don't compare the .RFT files by default if the .UNRST and .UNSMRY file checks succeed. I suppose that's reasonable, but that means we don't necessarily have a safety net for the .RFT file writer. Just out of curiosity, @akva2, how much work would it be to enable .RFT file comparisons for the cases that generate those output files?

@akva2
Copy link
Member

akva2 commented Nov 30, 2022

I'm a little confused by your statement of not comparing RFTs if unrst/unsmry checks succeeded. I find no such code. Afaict what happened here is that as there is no RFT file in the reference data, it will not be trigger a comparison, even if the new run has a RFT file. Or am I missing something?

@tklov11
Copy link
Contributor Author

tklov11 commented Dec 1, 2022

Hi, I can upload the .RFT file. Will that trigger a comparison?

@akva2
Copy link
Member

akva2 commented Dec 1, 2022

Hi, I will fix the infrastructure once I get around to it. We do not want manually uploaded reference data as it's machine sensitive and should be generated on the ci server.

@akva2
Copy link
Member

akva2 commented Dec 2, 2022

jenkins build this opm-common=3236 please

@akva2
Copy link
Member

akva2 commented Dec 2, 2022

@bska this flags an error if one case has a rft and the other does not which catches this. i can't think of any scenarios where we would not want to do so, but maybe I missed something. @tskille any particular reason why you ignored it before?

@bska
Copy link
Member

bska commented Dec 2, 2022

this flags an error if one case has a rft and the other does not which catches this.

By "this" I take it you mean OPM/opm-common#3236? If so, then that's a perfect solution to this.

i can't think of any scenarios where we would not want to do so, but maybe I missed something.

We definitely want to catch that case. Mostly because that signals either that we have new features or because something's gone terribly wrong in the simulation. That fact that the revised check also detects a situation that requires updating the reference solutions is an added bonus.

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