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

EclipseGrid Extented for LGR and Extended CARFIN Object to read and store parent's labels #4255

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

Conversation

arturcastiel
Copy link
Contributor

@arturcastiel arturcastiel commented Oct 11, 2024

  1. CARFIN reads and stores LGR parent name
  2. method num_parent_cells() returns the number of parent cells that were refined under the same label
  3. method parent_cellsIJK() return a tuple containing 3 lists I, J and K lists of the elements parents' elements refined.
  4. EclipseGrid Class has been extended to also describe LGR cells:
  • LGR Children are a collection of objects of the EclipseGridLGR a class type derived from EclipseGrid that appends information required by the LGR method.

  • EclipseGridLGR may also have as children other EclipseGridLGR, i.e., EclipseGrid is compatible with Nested LGR.

  • getActiveIndexLGR maps from LGR_Label and I,J,K onto the Comprised Index.

  • LgrTests.cpp test all these features.

…ls returns the number of parent cells that were refined under the same label 3 - method parent_cellsIJK() return I J K lists of the elements parents elements refined
@arturcastiel
Copy link
Contributor Author

jenkins build this please

@arturcastiel
Copy link
Contributor Author

Can someone please give me access to Jenkins?

@aritorto
Copy link
Member

aritorto commented Oct 11, 2024

I don't have that power, but at least can run jenkins now :)

I took a quick look and got the impression that we have a similar computation on opm-grid, but maybe it's necessary to have it here too. I'll take a deeper look next week!

@aritorto
Copy link
Member

jenkins build this please

@arturcastiel
Copy link
Contributor Author

Thank you, @aritorto !

opm/input/eclipse/EclipseState/Grid/Carfin.cpp Outdated Show resolved Hide resolved
opm/input/eclipse/EclipseState/Grid/Carfin.cpp Outdated Show resolved Hide resolved
opm/input/eclipse/EclipseState/Grid/Carfin.hpp Outdated Show resolved Hide resolved
opm/input/eclipse/EclipseState/Grid/Carfin.cpp Outdated Show resolved Hide resolved
opm/input/eclipse/EclipseState/Grid/Carfin.cpp Outdated Show resolved Hide resolved
opm/input/eclipse/EclipseState/Grid/Carfin.hpp Outdated Show resolved Hide resolved
opm/input/eclipse/EclipseState/Grid/Carfin.hpp Outdated Show resolved Hide resolved
… GLOBAL, and code is properly formated where needed
@arturcastiel
Copy link
Contributor Author

@akva2 thank you for your kind review. let me know if I got the alignment correct.

@akva2
Copy link
Member

akva2 commented Oct 14, 2024

yes, perfect. i have granted jenkins access.

@arturcastiel
Copy link
Contributor Author

jenkins build this please

@akva2
Copy link
Member

akva2 commented Oct 14, 2024

hmm. try again please.

@arturcastiel
Copy link
Contributor Author

jenkins build this please

@aritorto
Copy link
Member

The content of the PR looks correct to me. I just wonder where we need to use this. Could you maybe give me a hint?

If it's in a context where "the grid is not available", then it's perfect to have such methods (parent_cellsIJK(), and num_parent_cells()). Potentially, the analogous methods for refined cells would be needed too: num_refined_cells() is almost for free (NXxNYxNZ), refined_cellsIJK() require a tiny computation involving NX, NY, NZ, and the dimension of the block of parent cells (I2-I1, J2-J1, K2-K1).

If that's not the case ("the grid is available"), you can get these values through the grid's CartesianMapperIndexm or more general, LevelCartesianMapper (pending review/marge OPM/opm-grid#765 OPM/opm-grid#766 OPM/opm-simulators#5633 OPM/opm-simulators#5649)

@arturcastiel
Copy link
Contributor Author

@aritorto I am devising an extension to the EclipseGrid object that will treat and store the LGR refined cells hierarchically. The endgame here is to extend the save method to handle the output of LGR cells. The initialization of each individual COORD and ZCORN will be done after the grid is initialized. I hope to release a PR draft soon so you can take a look on it.

@aritorto
Copy link
Member

Thanks for the explanation @arturcastiel. I look forward to the coming draft PR!

- Introduced `EclipseGridLGR` class, derived from `EclipseGrid`
- Implemented a hierarchy to represent LGR cells within the EclipseGrid structure
- Improved organization and maintainability of the grid representation
@arturcastiel
Copy link
Contributor Author

arturcastiel commented Oct 14, 2024

this was supposed to be a draft to my local repository. for some reason I do not understand it got pushed to this PR. I will try to set the last commit as draft.

@arturcastiel arturcastiel marked this pull request as draft October 14, 2024 15:34
@arturcastiel arturcastiel marked this pull request as ready for review October 21, 2024 11:16
@arturcastiel
Copy link
Contributor Author

jenkins build this please

@aritorto
Copy link
Member

I've left some minor comments, but I will need more time to complete a detailed review. Thank you!

@arturcastiel arturcastiel changed the title Extended CARFIN Object to read and store parent's labels EclipseGrid Extented for LGR and Extended CARFIN Object to read and store parent's labels Oct 21, 2024
Copy link
Member

@akva2 akva2 left a comment

Choose a reason for hiding this comment

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

Mostly cosmetics but also a few code pattern questions.

opm/input/eclipse/EclipseState/EclipseState.cpp Outdated Show resolved Hide resolved
opm/input/eclipse/EclipseState/EclipseState.cpp Outdated Show resolved Hide resolved
opm/input/eclipse/EclipseState/Grid/EclipseGrid.cpp Outdated Show resolved Hide resolved
opm/input/eclipse/EclipseState/Grid/EclipseGrid.hpp Outdated Show resolved Hide resolved
opm/input/eclipse/EclipseState/Grid/EclipseGrid.hpp Outdated Show resolved Hide resolved
opm/input/eclipse/EclipseState/Grid/EclipseGrid.hpp Outdated Show resolved Hide resolved
opm/input/eclipse/EclipseState/Grid/EclipseGrid.hpp Outdated Show resolved Hide resolved
Copy link
Member

@aritorto aritorto left a comment

Choose a reason for hiding this comment

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

I haven't finished yet. I left a few more comments. Brief documentation would be very appreciated and might speed up the review process. Thanks!

opm/input/eclipse/EclipseState/Grid/EclipseGrid.cpp Outdated Show resolved Hide resolved
opm/input/eclipse/EclipseState/Grid/EclipseGrid.cpp Outdated Show resolved Hide resolved
opm/input/eclipse/EclipseState/Grid/EclipseGrid.cpp Outdated Show resolved Hide resolved
opm/input/eclipse/EclipseState/EclipseState.cpp Outdated Show resolved Hide resolved
opm/input/eclipse/EclipseState/EclipseState.cpp Outdated Show resolved Hide resolved
opm/input/eclipse/EclipseState/Grid/EclipseGrid.cpp Outdated Show resolved Hide resolved
opm/input/eclipse/EclipseState/Grid/EclipseGrid.cpp Outdated Show resolved Hide resolved
opm/input/eclipse/EclipseState/Grid/EclipseGrid.cpp Outdated Show resolved Hide resolved
Copy link
Member

@aritorto aritorto left a comment

Choose a reason for hiding this comment

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

A few more comments/questions

{
init_father_global();
lgr_label= self_label;
lgr_level = father_lgr_level + 1 ;
Copy link
Member

Choose a reason for hiding this comment

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

This might not be a problem for now, but... Dune's Grid interface follows this scheme where children of a cell ive in "parent cell level +1". However, currently, for CpGrid, "LGR1" is stored in level 1, "LGR2" stored in level 2 and so on... even though all parent cells belong to level zero.

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.

3 participants