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] Option to fix amount of kinetic reactant #2638

Merged
merged 4 commits into from Sep 13, 2019

Conversation

@joboog
Copy link
Contributor

joboog commented Aug 30, 2019

This pull request adds an option to fix the amount of a defined kinetic_reactant to its initial_amount. Subsequently the change of the amount of a kinetic_reactant in time step i does not affect the amount in subsequent time steps.
This is necessary when using a kinetic_reactant as reaction rate that only transfers mass between different reactants but does not represent a reactive species itself.

  1. Feature description was added to the changelog
  2. Tests covering your feature were added?
  3. [x ] Any new feature or behavior change was documented?
Copy link
Member

endJunction left a comment

Code needs clang-format, and fixes for the commented parts. Good otherwise.
Didn't read the text.

@renchao-lu

This comment has been minimized.

Copy link
Member

renchao-lu commented Aug 30, 2019

Need to discuss. Why not straightly use the initial amount of a kinetic reactant in the rate expression?

@renchao-lu

This comment has been minimized.

Copy link
Member

renchao-lu commented Aug 30, 2019

Thanks for your interpretation. Now I understand your motivation. Please add an additional check in the creation process.

@renchao-lu

This comment has been minimized.

Copy link
Member

renchao-lu commented Sep 2, 2019

Looks good to me. Can be merged after documentation review done by Vanessa. @montoyav
One more addition, please sort out your commits.

@montoyav

This comment has been minimized.

Copy link
Member

montoyav commented Sep 10, 2019

In general some additional points in the web description should be corrected/checked: a) Nomenclature should be consistent, b) reference to Samsó is missing, c) Figure with the kinetic reactions was not appearing, d) Figure 3 appears as Fig. 2, e) it is difficult to see from the animated figure which is the t = 0, maybe it will help to see how the time is progressing? f) it is mentioned that in the Figure it is shown the first 4 time steps, but it is not indicated how much time is this, g) simulation time was also not mentioned (20 days?).

@@ -121,3 +117,5 @@ Please note that due to the long computation time of the simulation, the corresp
Boog, J., Kalbacher, T., Nivala, J., Forquet, N., van Afferden, M., Müller, R.A., 2019. Modeling the relationship of aeration, oxygen transfer and treatment performance in aerated horizontal flow treatment wetlands. Water Research. 157 , 321 - 334. https://doi.org/10.1016/j.watres.2019.03.062.

Langergraber, G., Rousseau, D.P.L., García, J., Mena, J., 2009. CWM1: A general model to describe biokinetic processes in subsurface flow constructed wetlands. Water Science and Technology, 59 (9), 1687-1697. https://doi.org/10.2166/wst.2009.131.

Samso...

This comment has been minimized.

Copy link
@endJunction
Copy link
Member

TomFischer left a comment

After fixing the reference in the documentation.

@joboog joboog force-pushed the joboog:new_kineticreactant_fix_m branch from e388f08 to b37676b Sep 12, 2019
@joboog

This comment has been minimized.

Copy link
Contributor Author

joboog commented Sep 12, 2019

commits squashed and branch rebased.

@endJunction

This comment has been minimized.

Copy link
Member

endJunction commented Sep 12, 2019

Looks ok to me. When changlog entry is made, this can be merged.

@endJunction endJunction removed the WIP 🏗 label Sep 12, 2019
@montoyav

This comment has been minimized.

Copy link
Member

montoyav commented Sep 12, 2019

Good to me as well

@joboog

This comment has been minimized.

Copy link
Contributor Author

joboog commented Sep 13, 2019

@renchao-lu renchao-lu merged commit 3abbc4f into ufz:master Sep 13, 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 #20190912.11 succeeded
Details
@joboog joboog deleted the joboog:new_kineticreactant_fix_m branch Sep 13, 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.