-
Notifications
You must be signed in to change notification settings - Fork 14
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Restructure AddSourceTermsMultiGroup #734
Conversation
for more information, see https://pre-commit.ci
…b.com/quokka-astro/quokka into chong/seperate-single-and-multi-group
for more information, see https://pre-commit.ci
…b.com/quokka-astro/quokka into chong/seperate-single-and-multi-group
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
…ource_terms_multi_group.hpp.
Azure Pipelines successfully started running 2 pipeline(s). |
/azp run |
Azure Pipelines successfully started running 2 pipeline(s). |
for more information, see https://pre-commit.ci
/azp run |
Azure Pipelines successfully started running 2 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a good improvement and much better structured. I haven't gone through each line yet.
Did you intend to make the same changes to AddSourceTermsSingleGroup and AddSourceTermsMultiGroup? It looks like you've made similar changes, but I haven't compared them line-by-line.
There is a lot of commented-out code. Is it necessary to keep that? Otherwise, I think it would be a lot cleaner to just remove it.
No, I did not do the same thing to
I see only one commented-out code (just before using |
for more information, see https://pre-commit.ci
/azp run |
Azure Pipelines successfully started running 2 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good.
It would be useful to have the ordering of the variables used to define the Jacobian matrix listed in a comment next to the definition of JacobianResult
.
|
Description
In this PR I attempt to make the
AddSourceTermsMultiGroup
function more structured.Now,
AddSourceTermsMultiGroup
looks like this:and this is the only place where
enable_dust_gas_thermal_coupling_model_
is used.Next, hopefully, I can implement more complex dust models and cooling/heating by defining new
ComputeJacobian
and newComputeDustTemperature
.Related issues
N/A
Checklist
Before this pull request can be reviewed, all of these tasks should be completed. Denote completed tasks with an
x
inside the square brackets[ ]
in the Markdown source below:/azp run
.