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

Update MICM wrapper for species unit conversions #78

Merged
merged 14 commits into from
Feb 21, 2024

Conversation

boulderdaze
Copy link
Collaborator

@boulderdaze boulderdaze commented Jan 26, 2024

Closes #73

  • Added two unit conversion functions i.e. mol m-3 -> kg kg-1 and vise versa
  • Updated the meta data
  • Set up the test environment within the repository

@boulderdaze boulderdaze changed the title [Draft] Update MICM wrapper for species unit conversions Update MICM wrapper for species unit conversions Jan 30, 2024
@boulderdaze boulderdaze marked this pull request as ready for review January 30, 2024 03:30
Copy link
Collaborator

@nusbaume nusbaume left a comment

Choose a reason for hiding this comment

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

@boulderdaze Looks good! Just have some hopefully minor change requests.

Also I am happy to leave the final decision up to you, @mattldawson, and others, but I personally think we could incorporate the Github Action test you made in your fork into this repo. I did request some changes that might require modifying the test itself, and its unclear what our final testing framework will look like, but all else being equal I would rather get some test coverage in now then wait for a "perfect" system later. If you all agree then we can either add the test and Github action to this PR now or add it as a separate PR later (either way is fine with me). Thanks!

doc/ChangeLog Outdated Show resolved Hide resolved
musica/micm/micm.F90 Outdated Show resolved Hide resolved
Comment on lines 98 to 99
! TODO(jiwon) Checks the denominator is non zero
! this code needs to go when ccpp framework does the check
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it would be pretty easy to do this check in the constituents object itself during instantiation, or during the "set" call, as having a molar mass of zero is unphysical. What do you think @peverwhee? If so then maybe we can get this done sooner rather than later.

musica/micm/micm.F90 Outdated Show resolved Hide resolved
musica/micm/micm.F90 Outdated Show resolved Hide resolved
musica/micm/micm.meta Outdated Show resolved Hide resolved
@boulderdaze boulderdaze changed the title Update MICM wrapper for species unit conversions [Draft]Update MICM wrapper for species unit conversions Feb 1, 2024
@boulderdaze boulderdaze marked this pull request as draft February 1, 2024 20:10
@boulderdaze boulderdaze changed the title [Draft]Update MICM wrapper for species unit conversions Update MICM wrapper for species unit conversions Feb 3, 2024
@boulderdaze boulderdaze marked this pull request as ready for review February 3, 2024 01:45
@boulderdaze boulderdaze requested a review from nusbaume February 3, 2024 01:46
Copy link
Collaborator

@nusbaume nusbaume left a comment

Choose a reason for hiding this comment

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

Thanks for the new testing infrastructure @boulderdaze! I did have some more questions and suggestions, but hopefully nothing that causes any major difficulties.

.github/workflows/test.yaml Outdated Show resolved Hide resolved
.github/workflows/test.yaml Outdated Show resolved Hide resolved
doc/ChangeLog Show resolved Hide resolved
.dockerignore Outdated Show resolved Hide resolved
docker/Dockerfile.musica Outdated Show resolved Hide resolved
musica/micm/micm.F90 Outdated Show resolved Hide resolved
test/musica/micm/test_micm_api.F90 Outdated Show resolved Hide resolved
test/musica/micm/test_micm_api.F90 Show resolved Hide resolved
cmake/atmosphericphysicsConfig.cmake.in Outdated Show resolved Hide resolved
cmake/cmake_uninstall.cmake.in Outdated Show resolved Hide resolved
Copy link
Collaborator

@mattldawson mattldawson left a comment

Choose a reason for hiding this comment

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

looks great! I think having these tests started is going to be very helpful moving forward

Copy link
Collaborator

@nusbaume nusbaume left a comment

Choose a reason for hiding this comment

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

Everything looks great to me now, thanks again!

Copy link
Collaborator

@peverwhee peverwhee left a comment

Choose a reason for hiding this comment

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

Thanks Jiwon! A couple of small comments, but nothing that should hold up the PR.

musica/micm/micm.F90 Outdated Show resolved Hide resolved
test/musica/micm/test_micm_api.F90 Outdated Show resolved Hide resolved
@boulderdaze boulderdaze merged commit a95abf0 into main Feb 21, 2024
1 check passed
@boulderdaze boulderdaze deleted the 73-unit-conversion branch May 22, 2024 17:38
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.

Update MICM wrapper for species unit conversions
5 participants