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

Factor of molar_density missing in Alquimia wrapper for "mass_source" #72

Closed
SaubhagyaGT opened this issue Dec 22, 2020 · 9 comments
Closed
Milestone

Comments

@SaubhagyaGT
Copy link

There is a bug in the Alquimia wrapper when providing solute source through mass_source and geochemical condition. There is a factor of molar density of water missing. Attached picture contains the details. This factor is there in the boundary condition but not in mass_source.
@ATS_Ticket

@smolins
Copy link
Contributor

smolins commented Feb 9, 2021

Saubhagya,

I believe the qmass,source is in units of m/s. Can you point out where in the code you see this problem?
Thanks

Sergi

@ecoon
Copy link
Collaborator

ecoon commented Feb 24, 2021

Did this get solved?

@ecoon
Copy link
Collaborator

ecoon commented Feb 24, 2021

Ok, I think I see the confusion. If this is surface flow, we often use m/s as the mass source unit (e.g precipitation is typically recorded in m/s). But in the subsurface we assume that the mass source is in units of mol/m^3/s. And more generally, because our mass flow equation conserves mols of water, we require sources in mol/(volume unit)/s. So it is somewhat of an accident that this works as is for surface water.

I think I agree with Saubhagya here that we're missing a factor of 1/density, at least by default, and the user should have to tell us if they want to give a mass source in m/s.

@smolins
Copy link
Contributor

smolins commented Feb 24, 2021 via email

@saubhagya-gatech
Copy link
Contributor

Once this issue is closed and changed are merged in the master, the input file for one of the sorption regression test need to be updated. Value of molar density of water need to be changed from 1 to 55500.
https://github.com/saubhagya-gatech/ats-regression-tests/blob/7bd34016dbb8e00432427ccad358b0a26f1bc90c/07_reactive_transport/reactive_transport-subsurface_sorption_logical.xml#L402

@ecoon
Copy link
Collaborator

ecoon commented Apr 7, 2021

Note this test has now been merged into ats-regression-tests master, so we'll have to update the test when @dasvyat rt-fixes branch pull request comes in.

@ecoon
Copy link
Collaborator

ecoon commented May 6, 2021

@smolins Sergi, I just wanted to point out that this is now closed for real, which likely breaks your test. For surface flow and coupled reactive transport, the "water source in meters" must be set to false, and the units of the water source must be in [mol m^-2 s^-1]. I recognize that this may be a bit annoying, but it is the natural unit of the source (as every other PDE is defined this way). If I can, I will try to find a way to have the MPC check for this option, and ensure it is correctly set in the flow PK.

I now believe that setting "water source in meters" to default to true was a mistake, and hopefully will likely change that default to false soon. It is a bit hard to change defaults though, because it is hard to catch input files that depended upon it...

The easiest way to fix your test would be to simply create an evaluator, e.g.:

   <ParameterList name="water_source" type="ParameterList">
     <Parameter name="field evaluator type" type="string" value="multiplicative evaluator" />
     <Parameter name="evaluator dependencies" type="Array(string)" value="{surface-precipitation_rain, surface-molar_density_liquid}" />
   </ParameterList>

Or something similar that multiples your current source (in m/s) times the density (mol / m^3) to get the right units (mol / m^2 / s).

@ecoon
Copy link
Collaborator

ecoon commented May 6, 2021

Ok, maybe I'm backtracking on this and will add a matching option to transport "water source in meters". This thing must however default to false (because it makes no sense in the subsurface case, it cannot be true there). So if you're doing surface transport, you can set this to true manually, which matches the default in flow.

@smolins
Copy link
Contributor

smolins commented May 11, 2021 via email

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

No branches or pull requests

4 participants