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

[CL] Surface Sorption Process #2658

Merged
merged 19 commits into from Oct 23, 2019
Merged

Conversation

@renchao-lu
Copy link
Member

renchao-lu commented Sep 11, 2019

This pull request is motivated for surface sorption/desorption simulation. For this purpose, we further extend the existing ChemistryLib. The newly added utilities are summarized as follows:

  • Add keyword SURFACE for surface sorption/desorption simulation.
  • Add keyword KNOBS for specifying numerical settings used in speciation calculation.
  • Add keyword USER_PUNCH for output user-defined secondary variables in time-series vtu files.
  • Add keyword DUMP for acquisition of aqueous solutions of last time step.
  • Execute equilibrium calculation with regards to chemical systems in the initial phase.
  • Optimized current file-based work flow.
  1. Feature description was added to the changelog
  2. Tests covering your feature were added?
  3. Any new feature or behavior change was documented?
@renchao-lu renchao-lu force-pushed the renchao-lu:SurfacePropertyOld branch 3 times, most recently from c73b285 to a2a0007 Sep 11, 2019
@renchao-lu renchao-lu changed the title [Just for temporary build] Surface Sorption Process [CL] Surface Sorption Process Sep 18, 2019
@renchao-lu renchao-lu force-pushed the renchao-lu:SurfacePropertyOld branch from a2a0007 to b055383 Sep 21, 2019
@renchao-lu renchao-lu removed the WIP 🏗 label Sep 21, 2019
@renchao-lu renchao-lu force-pushed the renchao-lu:SurfacePropertyOld branch from b055383 to 32e7e9a Sep 21, 2019

# ESTRAL Thermodynamic Data Base
#
# Project: ESTRAL (FKZ 02 E 10528) Developed 22-Jul-2009

This comment has been minimized.

Copy link
@endJunction

endJunction Sep 22, 2019

Member

This file needs a clear origin and a license.

This comment has been minimized.

Copy link
@montoyav

montoyav Sep 29, 2019

Member

@renchao-lu @endJunction I take care of asking the group who provided us the database how they want we make reference to it.
General comment: In my opinion, chemical thermodynamic databases used by OGS to perform "reactive processes" should be more clearly documented, "visible". @endJunction maybe we speak about this and see what can be the best solution

@renchao-lu renchao-lu force-pushed the renchao-lu:SurfacePropertyOld branch from aed1969 to 0385ec7 Sep 23, 2019
struct SurfaceSite
{
SurfaceSite(std::string&& mineral_,
double const site_density_,

This comment has been minimized.

Copy link
@joboog

joboog Sep 23, 2019

Contributor

please add a variable for the flag "-sites".
In Phreeqc you can specifiy a surface site by giving the -site_density in n/m2 or by giving the absolute amount of surface sites by -sites.

This comment has been minimized.

Copy link
@renchao-lu

renchao-lu Sep 23, 2019

Author Member

Good point. I will follow your suggestion to implement further extension.

This comment has been minimized.

Copy link
@joboog

joboog Sep 24, 2019

Contributor

I would also suggest to stick to the Phreeqc flag names: mineral --> name.

This comment has been minimized.

Copy link
@renchao-lu

renchao-lu Sep 27, 2019

Author Member

I would also suggest to stick to the Phreeqc flag names: mineral --> name.

Changed.

? chemical_system_id + 1
: num_chemical_systems + chemical_system_id + 1;
os << "-equilibrate with solution " << aqueous_solution_id << "\n";
os << "-sites_units DENSITY" << "\n";

This comment has been minimized.

Copy link
@joboog

joboog Sep 23, 2019

Contributor

if you add a variable for the absolute amount of surface sites, defining the -sites_units here will be optional.
You need to add it only in case you define a surface using site_density in CreateSurface.cpp.

This comment has been minimized.

Copy link
@renchao-lu

renchao-lu Sep 27, 2019

Author Member

I prefer to keep the implementation as it is in this pull request, and set up another pull request for this extension. @joboog

This comment has been minimized.

Copy link
@joboog

joboog Sep 27, 2019

Contributor

ok.

os << "USE surface " << chemical_system_id + 1 << "\n";
os << "SAVE solution " << chemical_system_id + 1 << "\n";
}

This comment has been minimized.

Copy link
@joboog

joboog Sep 23, 2019

Contributor

Here you define the Phreeqc inputfile keyword blocs and call them later.
You could shorten this by:

  • define aqueous_solution
  • if (dump) define aqueous_solution_prev
  • USE solution none
  • END
  • USE solution chemical_system_id + 1
  • if (!equilibrium_phases.empty()) define equilibrium_phase
  • if (!kinetic_reactants.empty())define kinetics
  • if (!surface.empty()) define surface chemical_system_id + 1 (equilibrate with aqueous_solution_prev)
  • END

This comment has been minimized.

Copy link
@renchao-lu

renchao-lu Sep 27, 2019

Author Member

Very helpful. Improved as suggested.

@renchao-lu renchao-lu force-pushed the renchao-lu:SurfacePropertyOld branch from 0385ec7 to 1a57699 Sep 27, 2019
@renchao-lu

This comment has been minimized.

Copy link
Member Author

renchao-lu commented Sep 27, 2019

Remarks:
I updated the reference results of equilibrium phase benchmark, because I added an additional step for equilibrium calculation before jumping into the time loop. In order to consolidate if my modification is correct or not in terms of both physical and technical viewpoints, please take a look where I intrude in the function void TimeLoop::initialize(). @HBShaoUFZ @montoyav @joboog @endJunction @wenqing

{
BaseLib::RunTime time_phreeqc;
time_phreeqc.start();
_chemical_system->executeInitialCalculation(_process_solutions);

This comment has been minimized.

Copy link
@endJunction

endJunction Sep 27, 2019

Member

Just an idea; Can this initialization happen in the ComponentTransport::setInitialConditions?

This comment has been minimized.

Copy link
@renchao-lu

renchao-lu Sep 27, 2019

Author Member

Would be nice. Need some time to dig deeper.

This comment has been minimized.

Copy link
@joboog

joboog Sep 30, 2019

Contributor

For instance, if you like to transfer the ChemistryLib to another process, wouldn't it be more general to have to initialization as @renchao-lu implemented?
When calling from ComponentTransport::setInitialConditions you would have to define the function for other processes (Richards Flowetc) seperately, right?

This comment has been minimized.

Copy link
@montoyav

montoyav Sep 30, 2019

Member

Maybe, important to know 1) how much additional computational time is needed if an additional equilibrium step is included and 2) if this additional step give more accurate results or not.

This comment has been minimized.

Copy link
@joboog

joboog Sep 30, 2019

Contributor

The step added by @renchao-lu will just be called once during simulation start-up. It will not substantiall increase the overall computational cost.

This comment has been minimized.

Copy link
@renchao-lu

renchao-lu Oct 2, 2019

Author Member

Sorry for the late reply. I just come back. As Johannes pointed out, the added equilibrium calculation is executed once before time looping.

@renchao-lu renchao-lu force-pushed the renchao-lu:SurfacePropertyOld branch 2 times, most recently from 6689f64 to 7cd3b39 Oct 11, 2019
@renchao-lu

This comment has been minimized.

Copy link
Member Author

renchao-lu commented Oct 11, 2019

  • Call back the benchmark for testing and await consent of project partners.
  • Completed parameter documentation.
@renchao-lu renchao-lu force-pushed the renchao-lu:SurfacePropertyOld branch from 7cd3b39 to 4f13cab Oct 14, 2019
@renchao-lu

This comment has been minimized.

Copy link
Member Author

renchao-lu commented Oct 14, 2019

Cleaned up clang-tidy warnings, while there is still no room to suppress the number of warnings under the threshold...

ChemistryLib/PhreeqcIO.cpp Outdated Show resolved Hide resolved
ChemistryLib/PhreeqcIO.cpp Outdated Show resolved Hide resolved
ChemistryLib/PhreeqcIO.cpp Outdated Show resolved Hide resolved
ChemistryLib/PhreeqcIO.cpp Outdated Show resolved Hide resolved
Copy link
Member

TomFischer left a comment

After resolving some tiny things:

@renchao-lu renchao-lu force-pushed the renchao-lu:SurfacePropertyOld branch from f9d4a8c to 4d5d42f Oct 23, 2019
@renchao-lu renchao-lu force-pushed the renchao-lu:SurfacePropertyOld branch from 4d5d42f to 36f3d45 Oct 23, 2019
@renchao-lu renchao-lu force-pushed the renchao-lu:SurfacePropertyOld branch from 36f3d45 to 383ce8d Oct 23, 2019
@endJunction endJunction merged commit 561a2a2 into ufz:master Oct 23, 2019
3 checks passed
3 checks passed
continuous-integration/jenkins/pr-merge This commit looks good
Details
deploy/netlify Deploy preview ready!
Details
ufz.ogs #20191023.22 succeeded
Details
@renchao-lu renchao-lu deleted the renchao-lu:SurfacePropertyOld branch Oct 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.