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

allow coefficient for additive coag kernel to be set from spec file #187

Merged
merged 4 commits into from
Oct 10, 2024

Conversation

zdaq12
Copy link
Contributor

@zdaq12 zdaq12 commented Aug 4, 2023

Resolves #186

@slayoo slayoo requested a review from jcurtis2 August 4, 2023 17:36
Copy link
Collaborator

@jcurtis2 jcurtis2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While the code looks good and accomplishes the purpose, I am wondering if env_state_t is the place for this variable beta_1 to reside. While it was least disruptive to existing code to place there as the coagulation kernel functions already take the env_state_t, my possible alternative suggestion would be placing it in scenario_t (and then passing that structure around). I think one could argue that this scaling factor is pretty similar in purpose to the wall loss parameters used for chamber_t which is a member of scenario_t.

@mwest1066 any thoughts?

@mwest1066
Copy link
Contributor

Are we thinking of these parameters as being something point-wise in space that affects the coagulation kernel? E.g., something like a turbulent kinetic energy that might affect a coagulation kernel? If so, then I think that this is something which could vary in space and time, and so it does make sense to think of it as similar to a temperature or pressure, and hence to be included in env_t.

@@ -73,6 +73,8 @@ module pmc_env_state
real(kind=dp) :: solar_zenith_angle
!> Box height (m).
real(kind=dp) :: height
!> Scaling coefficient for additive coagulation kernel.
real(kind=dp) :: beta_1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like the name of this variable (it's very mysterious until you read the comment). Could we call it something like additive_kernel_coefficient instead?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed from beta_1 to additive_kernel_coefficient throughout code in 5177a46

@jcurtis2 jcurtis2 merged commit ecc26d3 into compdyn:master Oct 10, 2024
5 checks passed
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

Successfully merging this pull request may close these issues.

allow setting the value of additive kernel constant from a spec file
3 participants