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

restructure component models #647

Merged
merged 2 commits into from
Mar 8, 2024
Merged

restructure component models #647

merged 2 commits into from
Mar 8, 2024

Conversation

juliasloan25
Copy link
Member

@juliasloan25 juliasloan25 commented Feb 28, 2024

Purpose

separate required vs model-specific (internal) functions and objects in component model files

follows off of #644

Content

  • distinguish between coupler-required and model-specific functions in component model files
  • merge bucket_init.jl and bucket_utils.jl
  • rename component model files to <model>.jl
  • alphabetize get_field and update_field! methods for easier comparison/readability

  • I have read and checked the items on the review checklist.

@juliasloan25 juliasloan25 mentioned this pull request Feb 28, 2024
22 tasks
@juliasloan25 juliasloan25 marked this pull request as draft March 4, 2024 22:55
@juliasloan25 juliasloan25 force-pushed the js/restructure-models branch 2 times, most recently from b47c122 to 9c24dcc Compare March 6, 2024 02:00
@juliasloan25 juliasloan25 marked this pull request as ready for review March 7, 2024 00:55
@juliasloan25 juliasloan25 force-pushed the js/restructure-models branch from 9c24dcc to 18856c1 Compare March 7, 2024 01:02
@juliasloan25 juliasloan25 requested a review from LenkaNovak March 7, 2024 01:07
Copy link
Collaborator

@LenkaNovak LenkaNovak left a comment

Choose a reason for hiding this comment

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

It looks like the rebase removed some of the changes in the previous PR, e.g. Spaces -> ClimaCore.Spaces , and perhaps some of the imports. It may be good to look over the changes here just to double check. It would be good to check that there are no behavior changes both for slabplanet and AMIP before merging. Otherwise LGTM, thank you for doing this! Approving, but I'm happy to take a look again if needed.

experiments/AMIP/components/atmosphere/climaatmos.jl Outdated Show resolved Hide resolved

Extension of Interfacer.get_field to get the net TOA radiation, which is a sum of the
upward and downward longwave and shortwave radiation.
"""
Copy link
Collaborator

Choose a reason for hiding this comment

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

I noticed recently that sometimes the docstrings of extensions can sometimes cause the extensions to be ignored. It doesn't seem to be problematic here, but we should probably decide on a convention (Clima wide) as to how to deal with this. (not as part of this PR)

Copy link
Member Author

Choose a reason for hiding this comment

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

good point!

experiments/AMIP/components/atmosphere/climaatmos.jl Outdated Show resolved Hide resolved
experiments/AMIP/components/atmosphere/climaatmos.jl Outdated Show resolved Hide resolved
@juliasloan25 juliasloan25 force-pushed the js/restructure-models branch 2 times, most recently from 0a83515 to 0320449 Compare March 8, 2024 00:38
@juliasloan25 juliasloan25 force-pushed the js/restructure-models branch from 0320449 to 1e66ec0 Compare March 8, 2024 00:58
@juliasloan25 juliasloan25 enabled auto-merge March 8, 2024 00:58
@juliasloan25 juliasloan25 merged commit c76b42e into main Mar 8, 2024
6 of 7 checks passed
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.

2 participants