Register species from MICM #93
Conversation
mattldawson
requested review from
cacraigucar,
K20shores,
peverwhee,
nusbaume,
boulderdaze and
mwaxmonsky
There was a problem hiding this comment.
I've reviewed from a Fortran standpoint. @nusbaume and @peverwhee will need to review the interactions with the constituent object and the testing sections as I've not attempted to review them.
musica/util.F90
Outdated
| has_error_occurred = .true. | ||
| end function has_error_occurred | ||
|
|
||
| end module musica_ccpp_util |
There was a problem hiding this comment.
github is indicating that there is no "newline" at the end of this file. stackoverlow indicates that best practice is a newline should be present on the last line of a file.
There was a problem hiding this comment.
This solved the problem for this particular file. The newline issue occurs on most (but not all) of the files in this PR. Let me know if you don't see the red circle with the dash in it, and I can indicated each file with the problem
There was a problem hiding this comment.
I think they should all now have newlines at the end. Let me know if I missed any!
There was a problem hiding this comment.
Thanks for implementing the new register phase and memory test! However I do have some suggestions and questions, including a few questions which may (?) impact the overall design. Happy to discuss anything in more detail if you'd like!
Also, just an FYI that (at least for now) the PR will require and update to the ChangeLog before being fully merged:
https://github.com/ESCOMP/atmospheric_physics/wiki/Development-workflow#changelog
Thanks again!
| [ccpp-table-properties] | ||
| name = micm | ||
| name = musica | ||
| type = scheme | ||
| dynamic_constituent_routine = musica_register |
There was a problem hiding this comment.
@peverwhee might know this better than I do, but I think you'll need a dependencies line here to ensure that micm is properly linked to the musica interface in the CCPP framework:
dependencies = micm/micm.F90,util.F90
There was a problem hiding this comment.
I believe @mattldawson has added the dependencies line in his sandbox, but CAM-SIMA doesn't currently do anything with the dependencies (i.e. grab them during the build)
I've added an agenda item for tuesday's meeting.
There was a problem hiding this comment.
Thanks for letting me know @peverwhee! Happy to discuss this Tuesday, although I am guessing if the framework is able to provide the list of dependency files at code generation time it will probably be trivial (e.g. a handful of new lines) to get the correct info into CAM-SIMA for successful building/linking.
There was a problem hiding this comment.
After our discussion in the CAM-SIMA development meeting, I believe the plan is to wait until the constituents PR for the CCPP-framework is merged in (NCAR/ccpp-framework#549) and then create an issue to update the framework to provide absolute paths for dependencies in the ccpp_datatable.xml file. This will allow CAM-SIMA to include the dependencies in the build with minor modification. @nusbaume @peverwhee - is this correct?
There was a problem hiding this comment.
Yep, that sounds like the correct plan to me!
musica/util.F90
Outdated
| has_error_occurred = .true. | ||
| end function has_error_occurred | ||
|
|
||
| end module musica_ccpp_util |
There was a problem hiding this comment.
This solved the problem for this particular file. The newline issue occurs on most (but not all) of the files in this PR. Let me know if you don't see the red circle with the dash in it, and I can indicated each file with the problem
There was a problem hiding this comment.
@mattldawson - you found all the files with the missing newlines!
There was a problem hiding this comment.
@mattldawson this PR looks good to me! However I'll hold off on approving until we can get it working in CAM-SIMA (i.e. fix the dependencies issue in the framework). Of course if you'd like this PR in sooner than that just let me know. Thanks!
That's fine by me. There are some TUV-x-related developments in the pipeline, but hopefully the CAM-SIMA updates will be in before these are ready. |
|
closing and reopening on |
Registers constituents and sets needed constituent properties from MICM configuration.
Additionally:
musica_ccppmodule to act as a the CCPP interface and moves existing functionality to amusica_ccpp_micmmodule in preparation for adding TUV-x (we will usemusica_ccpp_as a prefix for all the MUSICA modules in this repo to prevent naming conflicts with MUSICA Fortran library modules)closes #74