Closed daw538 closed 1 year ago
Hi Daniel, Sorry I did not see this review request until now. Happy to discuss in person at the Friday meeting if that is of interest?
Hi Daniel, Sorry I did not see this review request until now. Happy to discuss in person at the Friday meeting if that is of interest?
Hi Penny, yes I think that might be worth it. Dan
I have re-run the trip tests for the latest commit c68238b, which pass on all cases except socrates_aquaplanet_cloud
.
Results for all of the test cases ran comparing https://github.com/ExeClim/Isca/commit/9560521e1ba5ce27a13786ffdcb16578d0bd00da and https://github.com/ExeClim/Isca/commit/c68238b483e24249f72bace40004509cac97b675 are as follows... axisymmetric : pass bucket_model : pass frierson : pass giant_planet : pass held_suarez : pass MiMA : pass realistic_continents_fixed_sst : pass realistic_continents_variable_qflux : pass socrates_aquaplanet : pass socrates_aquaplanet_cloud : fail top_down_test : pass variable_co2_grey : pass variable_co2_rrtm : pass ape_aquaplanet : pass Nightmare, some tests have failed
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 do_cloud_simple
in idealised_moist_physics_nml
so that it turns on the SimCloud cloud scheme when true, whilst the SPOOKIE scheme is activated with a new control do_cloud_spookie
. The default state of both of these options is false to preserve previous behaviour.
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.
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
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.
@daw538 - I think that we're done here now. I reckon we should just merge it in. I see @ntlewis is still requested for a review. Do you have time to do this @ntlewis or are you happy for us to go ahead? I'll try and fix the automated testing in #255
@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.
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.