QCEFF Table #545
QCEFF Table #545
Conversation
…ning size of table
… them to the variables in the qcf_table_data_type
…o that these type structs are only used in algorithm_info_mod
… read in correctly; removed rowheaders argument from subroutines where not needed
…bs_inc_info subroutine
…bs_error_info subroutine
…ools_mod to filter_main in filter_mod
…e top of algorithm_info_mod
…ork with log_qcf_info
currently the "lower bound only" test is failing because the upper < lower check happens always rather then only when you have two bounds
…ing invalid bounds otherwise missing_r8 -88888 value for the upper bound is "less than" the lower bound
|
Code to be removed: |
|
obs_kind is outdated terminology. qty (quantity) is the term that has replaced kind. |
kind is outdated terminolgy for quantity #545 (comment)
|
@hkershaw-brown great suggestions. I'll commit them after we change the author on the previous commits |
|
|
||
| ! The information arguments are all intent (inout). This means that if they are not set | ||
| ! here, they retain the default values from the assim_tools_mod namelist. Bounds don't exist | ||
| ! in that namelist, so default values are set in assim_tools_mod just before the call to here. |
There was a problem hiding this comment.
Do all these arguments need to be in inout?
obs_inc_info now sets the defaults for
filter_kind,
rectangular_quadrature
gaussian_likelihood_tails
sort_obs_inc
spread_restoration
bounded_below
bounded_above
lower_bound
upper_bound
There was a problem hiding this comment.
I don't think they need to be anymore. Previous to these changes, the defaults for all these were set in assim_tools_mod. The info for the bounds set right before the call to obs_inc_info (https://github.com/NCAR/DART/blob/6a5fbb6126d472a48a449ab6f13ff671c59bfb41/assimilation_code/modules/assimilation/assim_tools_mod.f90#L986C1-L988C51). I think we can definitely remove these lines linked above from assim_tools_mod and make the bounds info intent(out).
The other control options for obs_inc_info are a bit more difficult. filter_kind, sort_obs_inc, spread_restoration, rectangular_quadrature, and gaussian_likelihood_tails are being set to the values specified in the &assim_tools_nml section of input.nml, as this comment describes:
(https://github.com/NCAR/DART/blob/b46a922aacd9f2226d15eccb0de22368196f9078/assimilation_code/modules/assimilation/algorithm_info_mod.f90#L438C1-L443C88).
The defaults for these variables are used if they are not specified in the nml (https://github.com/NCAR/DART/blob/6a5fbb6126d472a48a449ab6f13ff671c59bfb41/assimilation_code/modules/assimilation/assim_tools_mod.f90#L146C1-L153C48)
Then we we get to the call to obs_inc_info, the values are changed to the defaults specified in obs_inc_info OR whatever is set in the qcf table. I think we can still set them to intent(out), but I think it is a bit weird to have our users set these options in both the qcf table and the assim_tools_nml. What do you think @hkershaw-brown ?
There was a problem hiding this comment.
After thinking more about this about this comment I made, I don't know if its bad that the users can set these options in both the &assim_tools_nml and in the qcf table
While we are setting the values for filter_kind in both the &assim_tools_nml and in the qcf table, the filter_kind read in from the qcf table is specific to observation space increments
But this leads me to a new question, which is if we have a value for filter_kind (or any of the other obs_inc_info options) set in the &assim_tools_nml that is not the default (EAKF = 1), do we want to use this filter_kind in obs_inc_info as the default instead of using EAKF? This would mean that by default, the filter_kind used for the observation space increments would match the filter_kind being used in assim_tools_mod
For example, if a user has filter_kind = 8 () set in their &assim_tools_nml, this would be passed in as the filter_kind argument to obs_inc_info, which would then use this value as the 'default' filter
I could see there being issues where a user sets rectangular_quadrature = .false. or something in the namelist, but they don’t include a qcf table. So it ends up changing the value of rectangular_quatrature to .true. (the default) when it calls obs_inc_info without the user expecting/wanting this to happen
| possible_filter_kind_ints(1) = 1 | ||
| possible_filter_kind_ints(2) = 2 | ||
| possible_filter_kind_ints(3) = 8 | ||
| possible_filter_kind_ints(4) = 11 | ||
| possible_filter_kind_ints(5) = 101 |
There was a problem hiding this comment.
Definite change:
These filter_kind parameters are already defined in this module
DART/assimilation_code/modules/assimilation/algorithm_info_mod.f90
Lines 30 to 38 in a4723e7
Don't redefine them here.
Convert the strings to the parameter value when reading the table, rather that doing the string comparison everytime.
There was a problem hiding this comment.
@hkershaw-brown I made this suggested change for both the dist_type and filter_kind. They are both now converted from string to integer in read_qcf_table and are set to integers in the types.
| lower_bound = qcf_table_data(QTY_loc(1))%obs_inc_info%lower_bound | ||
| upper_bound = qcf_table_data(QTY_loc(1))%obs_inc_info%upper_bound | ||
|
|
||
| endif | ||
|
|
||
| ! Only need to set these two for options the original RHF implementation | ||
| !!!rectangular_quadrature = .true. |
There was a problem hiding this comment.
Do we have enough information to set appropriate values for these:
! Only need to set these two for options the original RHF implementation
!!!rectangular_quadrature = .true.
!!!gaussian_likelihood_tails = .false.
There was a problem hiding this comment.
Not yet. I discussed this with Jeff, and he said to just make a note of it for now. I have it mentioned in the open issues section of this PR
There was a problem hiding this comment.
@jlaucar this is the second one - what should be done with rectangular_quadrature and gaussian_likelihood_tails?
|
|
||
| !use default values if qcf_table_filename is not in namelist | ||
| if (.not. qcf_table_listed) then | ||
| dist_type = BOUNDED_NORMAL_RH_DISTRIBUTION |
There was a problem hiding this comment.
This is just a question for my own understanding. Why is the default BOUNDED_NORMAL_RH_DISTRIBUTION
with no bounds,
why not have the default NORMAL_DISTRIBUTION?
There was a problem hiding this comment.
@jlaucar there are two comments/questions on this pull request that you need to have a look at and answer. This is one of them.
…bution Currently the algorithm_info_mod does not catch these on table read
|
Note on:
Possibly the local algorithm_info_mod.f90 was removed for your test but not committed, because the branch as-is |
@hkershaw-brown yes, cam-fv finished for me because I removed the algorithm_info_mod in work locally, but forgot to commit and push this change. This has now been pushed to the remote qcf_table branch. |
kind is outdated terminolgy for quantity #545 (comment)
Description:
Previously the QCF code required an algorithm_info_mod specific to the model, which meant editing algorithm_info_mod.f90 to specify which distribution should be used for which quantity.
This code implements a QCF input table, which reads in the algorithm info choices (QCF options) at runtime and stores them in algorithm_info_mod module storage.
This replaces the former functionality of algorithm_info_mod if statements with the table information.
The observation, state, and inflation variables are read in from a single table. Each field keeps its own column, having 28 total in the table.
The full list of QCF input options and information of the structure of the table can be found in the documentation at DART/guide/qcf_table.rst
More info on the background of the issue can be read in the specification here: https://docs.google.com/document/d/1MnvEFfgj5SfFbnIahGHwjy1XJ5IWBvPS8NB1nrIjc8k/edit
Fixes issue
Fixes #503
Types of changes
Documentation changes needed?
While I have included new documentation on how to use the input table at DART/guide/qcf_table.rst , we may want to edit Jeff’s documentation at https://docs.dart.ucar.edu/en/quantile_methods/models/lorenz_96_tracer_advection/work/readme.html to reflect the difference in workflow for the tests listed. I think we should also add to this page the need to include the &probit_transform_nml when running on quantile_methods.
Tests
Compiled and ran filter with full debugging flags with Intel, CCE, gfortran
Bitwise identical to quantile_methods, tested with Intel
Information on how to use the QCF input table with the quantile code is in the documentation at DART/guide/qcf_table.rst
build_everything now passes for all models:
A developer test for the table read is in progress.
Checklist for merging
Checklist for release
Testing Datasets
Open Issues/Questions
There are two input options for obs_inc_info (rectangular_quadrature and gaussian_likelihood_tails) that are only compatible with the original RHF implementation. Currently, these variables are unused in algorithm_info_mod.f90. These could either be removed from alg_info_mod and the namelist, or we can implement a conditional:
if (filter_kind == RHF) then
rectangular_quadrature: .true.
gaussian_likelihood_tails: .false.
DART/assimilation_code/modules/assimilation/algorithm_info_mod.f90
Line 233 in fc48d9d
Currently, the only check on the bounds that is implemented is a simple check to ensure that the lower bound is not less than the upper bound. Do we know if we want to put more explicit limits on the bounds?
There are differences in the formatting of log_qcf_info to dart_log.out with the cce compiler. This PR #491 describes this general issue with cce.
Currently, I am logging the headers of the QCF table to dart_log.out. Do we want these in the log? Similarly, is writing the data straight to the dart_log sufficient, or do we want to format this more cleanly (i.e. make it look like a table)?