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

hard-coded regions in mpc_coupled_transport are wrong #113

Open
ecoon opened this issue Jan 26, 2022 · 4 comments
Open

hard-coded regions in mpc_coupled_transport are wrong #113

ecoon opened this issue Jan 26, 2022 · 4 comments
Assignees
Labels
reactive-transport-roundup Low-hanging fruit for quick fixups in ATS Integrated Reactive Transport

Comments

@ecoon
Copy link
Collaborator

ecoon commented Jan 26, 2022

mpc_coupled_transport hard-codes the "surface" region as the coupling region for surface flow. Typically "surface" is a 3D face region in the subsurface mesh, and is not valid in the 2D surface mesh. Changes to Amanzi seem to have exposed this error in both ATS code and tests.

Instead, "surface domain" is typically used to indicate the entire 2D surface domain.

That said, this is not required, so this MPC should get this region name from the user (defaulting to "surface domain") rather than hard-coding it.

@ecoon
Copy link
Collaborator Author

ecoon commented Jan 26, 2022

I fixed the incorrect name, changing "surface" --> "surface domain".

This mpc still needs some cleanup:

mpc_coupled_transport.cc:33 -- what happens when we try to use transport on subdomains, and "domain" is not the subsurface? This cannot be hard-coded. We can expect the user to provide the PKs in the correct order, e.g. pk_order[0] is the surface or subsurface, but it isn't ok to select this based on a magic domain name.

line 113: hard-coded names must go

lines 162 & 173: ask the user for the names of these regions. They can default to "surface" and "surface domain", respectively, but they should be settable on the input spec.

@ecoon ecoon assigned ecoon and dasvyat and unassigned ecoon and dasvyat Jan 26, 2022
@ecoon
Copy link
Collaborator Author

ecoon commented Jan 26, 2022

Trying to assign @zexuanxu not sure why he isn't showing up

@zexuanxu
Copy link
Contributor

Just to confirm that I got this message and will look into it.

@ecoon
Copy link
Collaborator Author

ecoon commented May 9, 2023

Is this fixed now?

@ecoon ecoon added the reactive-transport-roundup Low-hanging fruit for quick fixups in ATS Integrated Reactive Transport label May 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
reactive-transport-roundup Low-hanging fruit for quick fixups in ATS Integrated Reactive Transport
Projects
None yet
Development

No branches or pull requests

3 participants