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 calculation of solar zenith angle and Earth–Sun distance #337

Merged

Conversation

mattldawson
Copy link
Collaborator

@mattldawson mattldawson commented Dec 13, 2024

Tag name (required for release branches):
Originator(s): Matt Dawson

Description (include the issue title, and the keyword ['closes', 'fixes', 'resolves'] followed by the issue number):

Adds the calculation of solar zenith angle and Earth–Sun distance and makes them available as CCPP standard named variables.

@nusbaume - I wasn't sure if I put the call to recalculate the orbital properties is correct. If it isn't let me know, and I can move it.

closes #328
Additionally:

  • adds some stubbed-out dependencies needed for MUSICA, until the actual values are available from other physics schemes or the host model
  • adds some testing infrastructure needed for unit tests of this code. Once Initial pFUnit Integration #326 is merged in, I can update this PR to use the new unit testing infrastructure

In draft until ESCOMP/atmospheric_physics#171 is merged in

Describe any changes made to build system:
none

Describe any changes made to the namelist:
none

List any changes to the defaults for the input datasets (e.g. boundary datasets):
none

List all files eliminated and why:
none

List all files added and what they do:

List all existing files that have been modified, and describe the changes:
(Helpful git command: git diff --name-status development...<your_branch_name>)

If there are new failures (compared to the test/existing-test-failures.txt file),
have them OK'd by the gatekeeper, note them here, and add them to the file.
If there are baseline differences, include the test and the reason for the
diff. What is the nature of the change? Roundoff?

derecho/intel/aux_sima:

derecho/gnu/aux_sima:

If this changes climate describe any run(s) done to evaluate the new
climate in enough detail that it(they) could be reproduced:

CAM-SIMA date used for the baseline comparison tests if different than latest:

mattldawson and others added 30 commits December 10, 2024 14:24
MPAS dynamical core can now integrate the dynamical states with time.
This functionality is intended for use by dynamics-physics coupling.
This helper function reverses the order of elements in a 1-d array.
This makes the assignments less error-prone and much more intuitive.
This subroutine provides a streamlined mechanism for exchanging constituent
states between CAM-SIMA and MPAS.
Use the new `dyn_exchange_constituent_state` subroutine to perform default
initialization for all constituents instead.
This assumption does not always hold.
mattldawson added a commit to ESCOMP/atmospheric_physics that referenced this pull request Dec 14, 2024
Adds the calculation of solar zenith angle and Earth-Sun distance needed
as inputs to TUV-x.

closes #163 
closes #164 

Both calculations required functions that are currently in shared code
in CAM
[here](https://github.com/ESCOMP/CESM_share/blob/f6f31fd61cb8f80aee97311fcca64b3e26b0202c/src/shr_orb_mod.F90#L736-L814)
and
[here](https://github.com/ESCOMP/CESM_share/blob/f6f31fd61cb8f80aee97311fcca64b3e26b0202c/src/shr_orb_mod.F90#L110-L159)
that I put in a modified form in the `to_be_ccppized/` folder. @nusbaume
@peverwhee - if there is a better way to handle these functions, let me
know and I can update the PR.

Requires an update to CAM-SIMA that is in review to make certain
variables available through CCPP
(ESCOMP/CAM-SIMA#325)

After more discussion, the plan is to use the shared functions in
CAM-SIMA and pass the outputs through CCPP. So, this PR will be put in
draft until this issue is finished:
ESCOMP/CAM-SIMA#328

A related draft PR to CAM-SIMA has been added
(ESCOMP/CAM-SIMA#337) and will be un-drafted
once this PR is merged in.
@mattldawson mattldawson marked this pull request as ready for review December 14, 2024 00:25
Copy link
Member

@jimmielin jimmielin left a comment

Choose a reason for hiding this comment

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

Thanks @mattldawson! Looks good to me, I have one request regarding allocate calls to be consistent with rest of SIMA code and just minor comments on the comments to clarify the purpose of musica_ccpp_dependencies.F90.


integer, intent(in) :: number_of_columns

allocate(solar_zenith_angle(number_of_columns), source=FILL_R8)
Copy link
Member

Choose a reason for hiding this comment

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

For these allocate calls (and elsewhere) could check_allocate (from cam_abortutils) be added after those as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think they are all updated now. Let me know if I missed any.

allocate(blackbody_temperature_at_surface(horizontal_dimension))
allocate(cloud_area_fraction(horizontal_dimension, vertical_layer_dimension))

surface_albedo(:) = 0.1_kind_phys
Copy link
Member

Choose a reason for hiding this comment

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

Just to confirm - these appear to be realistic values for a first version of MUSICA in CAM-SIMA, while the necessary physical quantities are not fully available yet. Is this correct?

Just to make sure since if "realistic" values are provided here at initialization it might be difficult to debug later when something's supposed to be provided from a physical scheme and the values are not as expected. But if this is a placeholder then it is fine!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is just a placeholder, but I tried to add more text to the comments describing this module to make clear that it should be entirely removed once the things that actually provide this information are in place, and before doing any real science. Let me know if it's more clear now, or if we should add anything else.

@@ -253,6 +255,14 @@ subroutine cam_init(caseid, ctitle, model_doi_url, &
! end if
call history_init_files(model_doi_url, caseid, ctitle)

! Temporary: Initialize MUSICA dependencies
Copy link
Member

Choose a reason for hiding this comment

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

Purely a comment nitpick - Can I ask if the comment could be made clearer here? "dependencies" is a generic term and I thought this was initializing external libraries, etc. Maybe it could be described as prescribing physical quantities necessary for MUSICA that are currently unavailable in SIMA? Something like that would make it clear that it's assigning placeholder values for an early version of MUSICA.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I updated the comment to use your suggested text.

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 getting this brought in @mattldawson! I just have a few change requests (but if any cause problems please let me know). Thanks!

src/control/cam_comp.F90 Outdated Show resolved Hide resolved
src/physics/utils/orbital_data.F90 Outdated Show resolved Hide resolved
src/physics/utils/orbital_data.meta Outdated Show resolved Hide resolved
test/unit/physics/utils/test_orbital_data_stubs.F90 Outdated Show resolved Hide resolved
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.

Looks good to me! I don't have any comments except for the ones others have mentioned

Copy link
Member

@jimmielin jimmielin 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 addressing my comments @mattldawson!

@mattldawson mattldawson requested a review from nusbaume December 16, 2024 18:52
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 good to me now, thanks @mattldawson!

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.

one tiny note - thanks @mattldawson

type = module
[solar_declination]
standard_name = solar_declination
units = radians
Copy link
Collaborator

Choose a reason for hiding this comment

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

in keeping with the SI units we (recently) decided on, this (and solar_zenith_angle) should be "rad".

if you want to just include this update in your existing PR - ESCOMP/atmospheric_physics#181 that would be swell.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

will do!

@mattldawson mattldawson merged commit 87f6fad into ESCOMP:development Dec 31, 2024
8 checks passed
@mattldawson mattldawson deleted the develop-328-orbital-properties branch December 31, 2024 19:53
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.

6 participants