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

add changes to fix the order #5630

Merged
merged 6 commits into from
Oct 1, 2024
Merged

Conversation

ElyesAhmed
Copy link
Contributor

This PR enables SFC (Space-Filling Curve) reordering in AluGrid as the default choice. The changes has been tested both with and without MPI across multiple cases from the opm-tests and opm-data!

enabling reordering as default
@akva2
Copy link
Member

akva2 commented Sep 25, 2024

Note that flipping these defines in opm code does nothing. You are entirely at the mercy of however dune-alugrid itself was built as the defines are not used in headers but in translation units.

@atgeirr
Copy link
Member

atgeirr commented Sep 25, 2024

Note that flipping these defines in opm code does nothing. You are entirely at the mercy of however dune-alugrid itself was built as the defines are not used in headers but in translation units.

This is an important point I was not aware of until today. Is there a way to detect either compile time or run time how dune-alugrid was built?

@akva2
Copy link
Member

akva2 commented Sep 25, 2024

No there is no indication of this. It's just an #ifdef in its gridfactory.cc that exits early. What makes it even more fun is that gridfactory.cc can be built inline (ie the .cc file is included in the .hh file) if you build dune-alugrid with COMPILE_ALUGRID_INLINE define. But you can't just set this for a library that was built without it as you'd get duplicate symbols. So as I said, at the mercy of how the library is built.

@atgeirr
Copy link
Member

atgeirr commented Sep 26, 2024

jenkins build this serial please

@atgeirr
Copy link
Member

atgeirr commented Sep 27, 2024

jenkins build this serial failure_report please

@ElyesAhmed
Copy link
Contributor Author

can we try again!

@akva2
Copy link
Member

akva2 commented Sep 30, 2024

jenkins build this serial failure_report update_data please

jenkins4opm pushed a commit to jenkins4opm/opm-tests that referenced this pull request Sep 30, 2024
Reason: PR OPM/opm-simulators#5630

opm-common     = 2a46477d267446991485e6ff8a13d2865c66d519
opm-grid       = 36069f06e5fe321c2dd161a7614dc95be5d05978
opm-models     = 6a0cb4b092f82c2a90193f1a16094c0fbdf503c3
opm-simulators = 6a0cb4b092f82c2a90193f1a16094c0fbdf503c3

### Changed Tests ###

  * spe12_alugrid
@akva2
Copy link
Member

akva2 commented Sep 30, 2024

jenkins build this serial opm-tests=1234 please

@atgeirr atgeirr added this to the Release 2024.10 milestone Sep 30, 2024
@atgeirr
Copy link
Member

atgeirr commented Sep 30, 2024

jenkins build this serial failure_report update_data please

@jenkins4opm
Copy link

Existing PR OPM/opm-tests#1234 was updated

jenkins4opm pushed a commit to jenkins4opm/opm-tests that referenced this pull request Sep 30, 2024
Reason: PR OPM/opm-simulators#5630

opm-common     = 12334dc673e68d09b94427e020632dfb7798b87d
opm-grid       = 36069f06e5fe321c2dd161a7614dc95be5d05978
opm-models     = 0abd925fd2a73fa8dabefd0579ea9875456f54be
opm-simulators = 0abd925fd2a73fa8dabefd0579ea9875456f54be

### Changed Tests ###

  * spe12_alugrid
@atgeirr
Copy link
Member

atgeirr commented Oct 1, 2024

jenkins build this serial opm-tests=1234 please

atgeirr added a commit to OPM/opm-tests that referenced this pull request Oct 1, 2024
@atgeirr
Copy link
Member

atgeirr commented Oct 1, 2024

Data update merged, will merge this once the data update Jenkins job has started.

@bska
Copy link
Member

bska commented Oct 1, 2024

will merge this once the data update Jenkins job has started.

The new reference solutions were installed 30 minutes ago. Did you accidentally forget to merge this?

@atgeirr atgeirr merged commit 47e7c7c into OPM:master Oct 1, 2024
1 check passed
@atgeirr
Copy link
Member

atgeirr commented Oct 1, 2024

Did you accidentally forget to merge this?

Yes, had to go to a meeting, sorry!

@blattms
Copy link
Member

blattms commented Oct 1, 2024

Just a note. According to my failed tests (because of missing rebase). The changes to the well solutions are quite large: https://ci.opm-project.org/job/opm-common-PR-builder/7522/testReport/(root)/mpi/compareECLFiles_flow_blackoil_alugrid_SPE1CASE2/

@atgeirr
Copy link
Member

atgeirr commented Oct 1, 2024

The changes to the well solutions are quite large:

We are aware of this, it is because the previous solution was wrong. The bugfixes in this PR apparently had a big influence.
@ElyesAhmed I hope I understood that correctly?

@blattms
Copy link
Member

blattms commented Oct 1, 2024

Well, our code should not rely on internal ordering. If it does then we are doomed (not only for ALUGrid...).

Maybe somebody could describe what this is supposed to fix. Where does this ordering matter?

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.

6 participants