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

Modified parallel plates permeability and updated discretization #2623

Merged
merged 21 commits into from
Sep 14, 2023

Conversation

karimifard
Copy link
Contributor

@karimifard karimifard commented Aug 17, 2023

  • Parallel plates permeability is now defined as ($a^2 /12$) instead of ($a^3 /12$) to be consistent with the other permeability models and the thermal discretization.
  • All TPFA discretizations are updated to be consistent with this change, this includes the update of stencil weights during the simulation when the hydraulic aperture changes.
  • In addition, the discretization for flow exchange between fracture and matrix is updated to account for fracture properties.
  • Small bug fix in Carman Kozeny permeability formula.

IntegratedTests PR: GEOS-DEV/integratedTests#37

@karimifard karimifard self-assigned this Aug 17, 2023
@karimifard karimifard marked this pull request as ready for review August 17, 2023 02:01
@karimifard karimifard changed the title Modified parallel plates perm v2 Modified parallel plates permeability and updated discretization Aug 17, 2023
@karimifard karimifard added flag: requires rebaseline Requires rebaseline branch in integratedTests type: feature New feature or request labels Aug 17, 2023
@CusiniM
Copy link
Collaborator

CusiniM commented Sep 5, 2023

@karimifard I have fixed that last compilation error on gpu machines. I am not too sure why but nvcc does not like whenever we call a device lambda nested inside a generic lambda. Maybe @klevzoff has a better idea of why that's not allowed. In any case, I have moved the kernel launches to FlowSolverBaseKernels.hpp and that seems to fix the issue on our gpu machine. If it passes the CI, I think this PR can be moved to the merge queue.

@klevzoff
Copy link
Contributor

klevzoff commented Sep 5, 2023

I am not too sure why but nvcc does not like whenever we call a device lambda nested inside a generic lambda. Maybe @klevzoff has a better idea of why that's not allowed.

I'm not too sure either, probably something to do with ABI and how the device lambda names are mangled. In any case, it's intentionally not supported by nvcc, so moving these kernel launches to a non-lambda context is the correct workaround.

@karimifard
Copy link
Contributor Author

Thanks @CusiniM @klevzoff everything seems to work 👍
Just for information (not related to this PR) the latest develop does not compile with clang anymore. I had to switch to gcc.

@herve-gross
Copy link
Contributor

Just for information (not related to this PR) the latest develop does not compile with clang anymore. I had to switch to gcc.

Same problem on my side, related to an explicit move operator requiring an explicit destructor:

In file included from /Users/hervegross/Documents/code/GEOS/src/coreComponents/dataRepository/WrapperBase.hpp:24:
/Users/hervegross/Documents/code/GEOS/src/coreComponents/dataRepository/xmlWrapper.hpp:160:3: error: explicitly defaulted move constructor is implicitly deleted [-Werror,-Wdefaulted-function-deleted]
  xmlDocument( xmlDocument && ) = default;
  ^
/Users/hervegross/Documents/code/GEOS/src/coreComponents/dataRepository/xmlWrapper.hpp:259:22: note: move constructor of 'xmlDocument' is implicitly deleted because field 'pugiDocument' has an inaccessible move constructor
  pugi::xml_document pugiDocument;

@CusiniM
Copy link
Collaborator

CusiniM commented Sep 6, 2023

Just for information (not related to this PR) the latest develop does not compile with clang anymore. I had to switch to gcc.

Same problem on my side, related to an explicit move operator requiring an explicit destructor:

In file included from /Users/hervegross/Documents/code/GEOS/src/coreComponents/dataRepository/WrapperBase.hpp:24:
/Users/hervegross/Documents/code/GEOS/src/coreComponents/dataRepository/xmlWrapper.hpp:160:3: error: explicitly defaulted move constructor is implicitly deleted [-Werror,-Wdefaulted-function-deleted]
  xmlDocument( xmlDocument && ) = default;
  ^
/Users/hervegross/Documents/code/GEOS/src/coreComponents/dataRepository/xmlWrapper.hpp:259:22: note: move constructor of 'xmlDocument' is implicitly deleted because field 'pugiDocument' has an inaccessible move constructor
  pugi::xml_document pugiDocument;

yeah, there is a fix for this: #2654

@CusiniM CusiniM merged commit 7f0065a into develop Sep 14, 2023
17 of 18 checks passed
@CusiniM CusiniM deleted the feature/karimifard/ModifiedParallelPlatesPermV2 branch September 14, 2023 02:48
@liyangrock liyangrock mentioned this pull request Jan 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flag: ready for review flag: requires rebaseline Requires rebaseline branch in integratedTests type: feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants