Add capability for user-defined parameter sets, with an example #172
Conversation
|
I guess this PR depends on #171, but that can be removed. |
There was a problem hiding this comment.
why don't we literate it and add it in the docs?
There was a problem hiding this comment.
That's what I asked about in the original post
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #172 +/- ##
=======================================
Coverage 90.93% 90.93%
=======================================
Files 10 10
Lines 1180 1180
=======================================
Hits 1073 1073
Misses 107 107 ☔ View full report in Codecov by Sentry. |
| For more information see [reference docs]. | ||
| """ | ||
| function ConstitutiveParameters(FT = Float64; | ||
| gas_constant = 8.3144598, |
There was a problem hiding this comment.
I had some discussions a while back with SEs asking if the parameters constructors should come with default values. The feedback I got was leaning towards not having the defaults, unless it is really beneficial to the structure of the code.
From what I understood the defaults should live in a toml file. - I don't have any opinions on this personally. More of a question of convention and how much we want to stick to it in different examples
There was a problem hiding this comment.
This is a "classroom example" that illustrates some thermodynamic calculations. I think we want to include examples like this in the docs as well to form a link between theromdynamic theory explained in the docs and functions that one calls to make thermodynamic calculations.
This example is not intended to illustrate how to use this package within an Earth system model. So, I understand the validity of the argument to avoid defaults in parameter structs that are designed for use in ClimaAtmos.
However, this parameter struct is not for ClimaAtmos or an Earth system model --- it's just for this small example. For this use case, it's nice to have the defaults here so that we can read this file and understand what the code is doing. The same applies for code snippets embedded in the documentation, I think.
|
Can you rebase this PR @glwagner? I’d definitely like these changes |
|
@glwagner, can you re-open this on branch off the main branch? (not the fork), I wasn't able to rebase it |
|
I can do that. |
|
@charleskawczynski I think we just need to run the formatter |
| colorrange = prange, | ||
| colormap = pmap, | ||
| alpha = 0.1, | ||
| ) |
There was a problem hiding this comment.
@charleskawczynski what are the rules about function arguments on one line vs many lines?
There was a problem hiding this comment.
I want to clean this up if possible within the rules that the formatter dictates (perhaps its not possible)
There was a problem hiding this comment.
The join_lines_based_on_source might be relevant? You might need to set this to true.
There was a problem hiding this comment.
If you want local custom formatting, you can do
#! format: off
custom formatted code
#! format: on
There was a problem hiding this comment.
Sometimes I find this very handy
|
@charleskawczynski looks like the buildkite isn't running even though I added the "Launch buildkite" label |
charleskawczynski
added
Launch Buildkite
Let me try. If it works, I'll see if I can update your privileges. |
|
@glwagner, maybe because it's a PR from a fork? Ah, or the branch needs to be rebased (so that it picks up the latest buildkite config). |
|
Superceded by #210 |
This PR tweaks the internal API for defining parameter sets to support user-defined parameters. This is important, because it will allow users to invoke functions in
Thermodynamics.jlwithout requiring the full suite of parameters inThermodynamicsParameters. For example, if one is only interested in computing density as a function of temperature, pressure, and (total) specific humidity, then they only require the universal gas constant and the molar masses of air and water.Next, this PR adds an example which illustrates such a parameter set, called
ConstitutiveParameters. It culminates in this plot:which uses sea level data derived from the JRA55 reanalysis (subsampled to make the dataset smaller), showing the primary dependence of density on temperature, secondary dependence on pressure, and teritiary dependence on specific humidity for the typical conditions of the Earth's atmosphere at sea level.
Currently, there are no examples, so this PR introduces the first one. I'd be happy to turn the example into something that appears in the documentation via Literate (the example is very close to that form already).