Skip to content

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

Merged
jcurtis2 merged 4 commits into
compdyn:masterfrom
zdaq12:set_additive_kernel_param
Oct 10, 2024
Merged

allow coefficient for additive coag kernel to be set from spec file#187
jcurtis2 merged 4 commits into
compdyn:masterfrom
zdaq12:set_additive_kernel_param

Conversation

@zdaq12

@zdaq12 zdaq12 commented Aug 4, 2023

Copy link
Copy Markdown
Contributor

Resolves #186

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

@jcurtis2 jcurtis2 left a comment

Copy link
Copy Markdown
Collaborator

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
Copy Markdown
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.

Comment thread src/env_state.F90 Outdated
!> Box height (m).
real(kind=dp) :: height
!> Scaling coefficient for additive coagulation kernel.
real(kind=dp) :: beta_1

Copy link
Copy Markdown
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
Copy Markdown
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
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