-
Notifications
You must be signed in to change notification settings - Fork 19
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
Register species from MICM #93
Conversation
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 like the change to a musica_ccpp_*
interface
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.
Looks good! Just have a couple questions from curiosity. I also like musica_ccpp_*
interface.
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'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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done!
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mattldawson - you found all the files with the missing newlines!
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.
@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_ccpp
module to act as a the CCPP interface and moves existing functionality to amusica_ccpp_micm
module 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