Closed jatkinson1000 closed 1 year ago
At the moment there are many variables that are required for this NN to run in SAM. Many of these are defined as global, so it's not immediately clear what is/isn't vital and how it should be used. In order to generalise this to other models (i.e. SCAM/CAM) we need to settle on a standardised set of inputs (e.g. how to represent moisture content, pressure etc.)
The model also requires information about the grid domain etc. and various physical constants. This previously made use of global variables in SAM, but we should discuss how we want to handle this for the NN as a standalone module. This will likely be through some kind of interface module of variables to be populated by the user. There will also need to be some sort of interface to convert variables from a given model to those used by the NN (Paul touched on this at his M2LiNES talk yesterday).
Another question is on the logical configuration of the NN - there are a number of logical variables that can be switched on/off to decide which input features to use. If we are using a single fixed implementation of the net then we can do away with these. Or do we expect we will need these?
This will require discussion with @judithberner and @paogorman along with @dorchard, @mondus and @jdenholm My suggestion would be that we move towards restructuring with a SAM interface here that can then be adapted for a SCAM/CAM interface in another PR to address #2 and #18
After discussion with @dorchard we concluded:
There are some big commented out blocks, I think due to the us dead-code removing code paths that won't be used becauserf_uses_rh
is .false.
. Should we remove these in this PR totally then?
There are some big commented out blocks, I think due to the us dead-code removing code paths that won't be used because
rf_uses_rh
is.false.
. Should we remove these in this PR totally then?
I think the block can perhaps be removed from the SAM interface code, but am inclined to leave them in the parameterisation as it's one main block and functions at the end.
Perhaps something to ask Paul on Thursday about how serious they are to re-implement in future?
Other option would be to open a separate issue to remove rf_uses_rh
code so that we can easily find the commit where it was present in future.
I have implemented the 3-layer structure, cleaned the code, and got rid of any implicit dependency on SAM.
These is still some physics to clarify, but this does not affect operation, so I'd like review on this now to merge and move to interfacing with SAM to check it still works OK. This will also allow @raehik to look into CamFORT etc.
Moving from draft to review.
This PR will also close #10 and #36 and potentially #35
This will now close #40
Useful discussion with @paogorman who answered a number of questions I had around the science/numerics. These will inform the refactor and as a result I will be making a few more (notable) changes on this PR so will likely want a re-review soon. Summarised below:
Questions:
q
is total water, and qn
is cloud water.
However, both are updated with adv + tot + sed, so what is the difference?
There are different checks on physical correctness (q checked at each addition, qn just checked after all added)?
q
is total water content (vapour plus qn
), qn
is non-vapour water content.
We can probably comment out qn
as it comes from SAM setup - in larger models it will be immediately precipitated out during a larger timestep.
In fact, just delete qn
!!
gt lt misses 0.0 -> can we add?
Think about this, likely negligible effect from changing in reality
Check variable names and units
Seems OK
tendency is $\Delta\psi$?
Yes. Itr would be nicer if we can refactor for tendencies to actually be tendencies rather than changes
Flux is what related to irhoadzdz
?
SAM equations are of the form:
$\text{tendency} = \text{d} h / \text{d}t = - 1 / \rho \text{d} (\rho u h) / \text{d}x = - 1 / \rho \text{d}(\text{flux}) / \text{d}x$
So the irhoadzdz
term comes from rearranging to get:
$\text{d}h = - \text{irhoadzdz} * \text{d}({flux})$
What is _tot
?
tot is a legacy from when the variable was total change. Now the tendencies are checked separately and this should really be called auto
CAM operates at the level of tendencies.
careful about removing dt as there is a dtn in irhodzdz
t is energy / cp
beware of this, especially in the SAM-CAM conversion
This is a fairly sizeable PR with several linked issues at this point. I propose we merge this back (after SAM integration check) and then open a subsequent PR linked to issues listed in the above comment refactoring further towards CAM interfacing by changing outputs to be tendencies rather than updated variables. Thoughts @raehik @dorchard ?
This PR should address and close #9 and close #10 and close #36 and close #40 and maybe close #35
nn_convection_flux
It also takes steps towards addressing #10 by adding a Makefile. However, there should be discussion over the build process we wish to use. The makefile currently requires a user to specify the location of NetCDF libraries to link against.