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

Add multi-grid cell solving to MUSICA API for MICM #192

Merged
merged 9 commits into from
Aug 13, 2024

Conversation

mattldawson
Copy link
Collaborator

@mattldawson mattldawson commented Aug 8, 2024

Adds the ability to solve multiple grid cells with a single call to the MICM solver. Includes updates to the Fortran and Python wrappers. Also, modifies the Chapman test conditions to avoid convergence failures.

closes #175

I don't really like the fact that these function calls accept pointers to 2D data for temperature, pressure, concentrations, etc. where the size and shape are assumed to be correct with no checks. I suggest that we create a separate issue to address this.

@mattldawson mattldawson requested review from boulderdaze, K20shores and dwfncar and removed request for boulderdaze and K20shores August 9, 2024 23:13
@mattldawson mattldawson marked this pull request as ready for review August 9, 2024 23:13
Copy link
Collaborator

@boulderdaze boulderdaze left a comment

Choose a reason for hiding this comment

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

I really like what you did! Your PR closes #156 and partially completes #178 (maybe this is enough?)
Your test coverage looks impressive. Looks like incorrect mapping between the species and concentrations caused the convergence failure, did it?

fortran/micm.F90 Show resolved Hide resolved
configs/chapman/reactions.json Show resolved Hide resolved
Comment on lines +10 to +19
"type": "USER_DEFINED",
"MUSICA name" : "reaction 2",
"reactants": {"E": {"qty": 1}},
"products": {"F": {"yield": 1}}
},
{
"type": "USER_DEFINED",
"MUSICA name" : "reaction 1",
"reactants": {"D": {"qty": 1}},
"products": {"E": {"yield": 1}}
Copy link
Collaborator

Choose a reason for hiding this comment

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

My only concern with this is that USER_DEFINED isn't technically part of the camp configuration. However, I do think this is incredibly useful. We could remove photolysis, first order loss, emission, from the technical specification of the configuration because they are all just logical versions of user defined and it allows us to not have to do funny things with music box for integrated reaction rates.

I guess there's nothing actionable with that paragraph; I'm just putting thoughts out there

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do you think we should add USER_DEFINED to the CAMP configuration? I agree, it might make sense to replace the photolysis, emissions and first-order loss with this one type.

@mattldawson mattldawson linked an issue Aug 12, 2024 that may be closed by this pull request
@mattldawson
Copy link
Collaborator Author

I really like what you did! Your PR closes #156 and partially completes #178 (maybe this is enough?) Your test coverage looks impressive. Looks like incorrect mapping between the species and concentrations caused the convergence failure, did it?

Thanks! I added #156 to the issues this closes. I think it might be enough for #178 also. If you agree, I can include it in the list.

@boulderdaze
Copy link
Collaborator

I really like what you did! Your PR closes #156 and partially completes #178 (maybe this is enough?) Your test coverage looks impressive. Looks like incorrect mapping between the species and concentrations caused the convergence failure, did it?

Thanks! I added #156 to the issues this closes. I think it might be enough for #178 also. If you agree, I can include it in the list.

Sounds good, I agree with you

This was referenced Aug 12, 2024
Copy link
Contributor

@dwfncar dwfncar left a comment

Choose a reason for hiding this comment

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

Will update 162_euler_option branch with the changes to MicmSolve.

@mattldawson mattldawson merged commit a030011 into main Aug 13, 2024
61 checks passed
@mattldawson mattldawson deleted the develop-175-multi-cell branch August 13, 2024 15:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants