-
Notifications
You must be signed in to change notification settings - Fork 127
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
Simcloud: A simple cloud scheme for Isca #246
Conversation
…near relationship with RH
…tion diagnosis and critic RH profiles
…itted critical RH profiles
Conflicts: src/atmos_spectral/driver/solo/idealized_moist_phys.F90
…into simple_clouds_diag_cf
…into simple_clouds_diag_cf
…here it changes with temperature
…) in AMIP simulations
…n random-overlap overlap assumption
Hi Penny, yes I think that might be worth it. Dan |
…d controls for each scheme so both cannot be used.
I have re-run the trip tests for the latest commit c68238b, which pass on all cases except
The failure behaviour is expected since I have changed the namelist parameter associated with using the available cloud schemes. The master commit does not have this option in the source so the associated run for this experiment fails. I have independently tested the equivalent runs from 9560521 and c68238b using the methodology from the trip test function which reports a pass. The changes to the namelist include changing the default behaviour of The current failure in the github test run is a result of change separate to this PR and is being addressed in #247 . An additional trip test for the SimCloud scheme will be provided in a future PR. |
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.
Hi Dan, great job on getting this into review and making both cloud schemes available. As we discussed in the group meeting, my review has been focused on the technical and readability of the code. In time it would have good to do a more rather focused science review but that is far from a small thing to do. In general the quality of the code is good so most of the suggestions I have made are things you can choose to do or not. Let me know if you don't agree with anything or don't understand my comments.
}, | ||
|
||
'diffusivity_nml': { | ||
'do_entrain': True, #False, |
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 ran my test cases with this as True but don't recall why. Noted here in case helpful.
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'm not sure either. Looking at the source the default is set to True
but it is not used in a number of the other test cases.
'depth': 20.0, # Depth of mixed layer used | ||
'land_option': 'input', # Tell mixed layer to get land mask from input file | ||
'land_h_capacity_prefactor': 0.1, # What factor to multiply mixed-layer depth by over land. | ||
'albedo_value': 0.12, # Ocean albedo value |
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.
albedo looks low to me. Is this what Qun used?
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.
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 wonder if this is because the normal mixed-layer albedo is so high precisely because we don't have clouds. But once we add clouds we don't need the surface albedo to be quite so high.
|
||
end subroutine large_scale_cloud_init | ||
|
||
subroutine large_scale_cloud_diag(pfull, ps, rh, q_hum, qsat, qcl_rad, wg_full, cf, Time) |
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 found the '_diag' routine name a bit misleading. In this context it means diagnosing clouds in the column, but it reads as though we are initialising diagnostics or similar. If it is not too much of a pain, could we change this to 'large_scale_cloud'
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 fine to me. This part of the code was easier to follow as the comments are much better. As with the cloud_simple.F90 I can't comment too much on the science parts without a lot more time to investigate, but it all looks superficially fine to me (i.e. nothing jumps out).
@@ -0,0 +1,920 @@ | |||
! The original code is from http://romps.berkeley.edu/papers/pubdata/2016/lcl/lcl.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.
Can you check that we can reproduce this code in here? I.e. take a look at the license Romps uses. I also see that there are other functions with different licenses in here. Can you make sure they are all open enough for us to use too please?
Given it was taken directly from Romps (which is a very trustworthy source), I propose we don't make any changes)
|
||
end subroutine marine_strat_cloud_init | ||
|
||
subroutine marine_strat_cloud_diag(temp, p_full, p_half, z_full, rh, q_hum, temp_2m, & |
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.
Same criticism here about diag being misleading. Propose 'marine_strat_cloud'
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.
There is quite a bit of science in this one, which probably should be reviewed in more detail than I have, but I don't have a week to spend on this PR so I am giving it the benefit of the doubt.
I am accepting this merge, in advance of going on mat leave, so that I do not hold the process up. I pass the review over to Neil and Stephen who can continue discussion with Dan. The issues with the PR I have raised have been discussed with Dan, and I am satisfied there is nothing preventing this from going onto the master (majority of comments are nice to have suggestions but not needed per say). Penny |
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.
Sorry I closed the PR rather than approved it. Have reopened and now accepting the PR.
Hi there! Can I ask if SimCloud passes its properties to RRTM? Or does RRTM currently just do clear-sky radiation? |
At the moment Simcloud just works in Socrates. We did try to get some cloud stuff working for RRTM, but we didn't finish it as we just switched to Socrates instead. |
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 all looks good to me. I haven't checked all of it massively closely, but I think it's important we get this in soon. If it's not breaking the trip tests then that's fine. Given that the master hasn't changed since they were run, I think it's fine to put this in. Thanks for all your work on this Dan. I guess one thing that should be done at some point would be to add a bit to the docs about the clouds. That could be left as a to-do!
'depth': 20.0, # Depth of mixed layer used | ||
'land_option': 'input', # Tell mixed layer to get land mask from input file | ||
'land_h_capacity_prefactor': 0.1, # What factor to multiply mixed-layer depth by over land. | ||
'albedo_value': 0.12, # Ocean albedo value |
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 wonder if this is because the normal mixed-layer albedo is so high precisely because we don't have clouds. But once we add clouds we don't need the surface albedo to be quite so high.
@@ -74,12 +74,18 @@ def get_nml_diag(test_case_name): | |||
input_files = exp_temp.inputfiles | |||
nml_out = exp_temp.namelist | |||
|
|||
#if 'soc_realistic_continents_fixed_sst_with_linear_cld_scheme' in test_case_name: |
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.
Given that the trip tests seem to have passed for the relevant cases, let's add these tests back.
@sit23 I'm happy for this to go in. Also, regarding the albedo issue above, I think it's low because clouds have been introduced. For example, a typical albedo for open ocean would be something like 0.08, so it's still higher than that. Given that the land prefactor is also > 1, I think this'll end up with a sensible albedo once the effect of clouds is included. |
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.
Happy for this to go in. See comment above about albedo.
SimCloud is a simple cloud diagnostic scheme for general circulation models, implemented previously by @lqxyz for Isca and described in Liu et al. 2021. Currently the model sits as a private branch -- this PR brings the scheme inline with the current master in order to merge into the master, giving Isca a full cloud scheme that supersedes the simpler SPOOKIE-2 derived scheme currently in the master.
I propose adding an additional test case to the current suite to provide an example of SimCloud in situ (trip test file would need updating which I have not yet done). Qun has provided one in his branch which can be used. I also propose either moving or removing the two SPOOKIE-2 cloud test cases that are currently present in the SOCRATES test case directory to avoid confusion.
There is one change to the file
mixed_layer.F90
that will need resolving. The present master has ice-dependent albedo calculated in the subroutinealbedo_calc
via the following method:In this situation all the trip tests pass as shown below.
In the PR branch however, this calculation has been modified from a step function (as above) to a ramp function, entirely replacing the where statement thus
and the following section was additionally commented out (in the current state of the PR I have reverted this behaviour)
Using the ramp function rather than the step function causes the trip tests for
realistic_continents_fixed_sst
andrealistic_continents_variable_qflux
to fail, which is understandable. We hence need to decide how to resolve the potential conflict between the two methods: do we bin one or do we introduce some form of choice with a namelist parameter? What would be the justification for using either?Once this PR has been accepted I will follow up with a secondary PR that updates the docs to include the cloud module and a list of the available test cases (which is currently present but hidden in the Isca/exp folder.