NCAR / DART

Data Assimilation Research Testbed
https://dart.ucar.edu/
Apache License 2.0
198 stars 145 forks source link

KQCEF #706

Closed iangrooms closed 3 months ago

iangrooms commented 4 months ago

Description:

Types of changes

Documentation changes needed?

Tests

As noted above, many of the new methods have unit tests. I have also used this code in this paper. Finally, I have run both the Lorenz-63 and Lorenz-96 models with the new filter (and with the associated probit transforms) on this branch and verified that they perform as well as or better than the EAKF (though slower).

Checklist for merging

Checklist for release

Testing Datasets

iangrooms commented 4 months ago

I'm not sure how to do it. Is that on a separate repository? Sorry for the dumb question - first time PR to DART.

hkershaw-brown commented 4 months ago

I'm not sure how to do it. Is that on a separate repository? Sorry for the dumb question - first time PR to DART.

no problem, thanks for this pull request btw!

The documentation is in this repo, its the .rst files, e.g.: DART/guide/qceff_probit.rst

They get turned into html via readthedocs & sphinx, so for this pull request the docs are available to view here: https://dart-documentation--706.org.readthedocs.build/en/706/. They'll rebuild when you commit to your branch.

If you want to build docs locally you can (see building locally)

Or if you have the documentation in text, but don't want to get involved writing .rst we can put them in .rst format.

Cheers, Helen

iangrooms commented 4 months ago

Excellent - thanks for the explanation. I've use .rst before so I'll try to add the documentation.

mjs2369 commented 4 months ago

Thanks so much for this @iangrooms !

I have just a couple small comments. The first is that I saw that you have a commit where you move obs_increment_kde out of assim_tools_mod and into kde_distribution_mod.f90. What is the reasoning behind this decision since all of the other obs_increment_x subroutines for the various filter kinds are still located in the assim_tools_mod?

The second is similar to Helen's question on the naming of KERNEL_QCEF - will we want to replace the old filter_kind KERNEL with this new code (i.e. remove obs_increment_kernel and other relevant code) or keep both filter_kinds? @hkershaw-brown what are your thoughts on this?

iangrooms commented 3 months ago

I saw that you have a commit where you move obs_increment_kde out of assim_tools_mod and into kde_distribution_mod.f90. What is the reasoning behind this decision since all of the other obs_increment_x subroutines for the various filter kinds are still located in the assim_tools_mod?

@jlaucar suggested that particular change. I think the idea is that subroutines that are closely tied to a specific distribution should be located within that distribution's module.

I'll go with whatever naming convention you prefer. The KERNEL filter kind is fundamentally different from the new KERNEL_QCEF - the new one is not a better version of the old one, so it makes more sense to me to keep both kinds. KERNEL is more like a Gaussian Mixture following Anderson & Anderson 1999 whereas the new KERNEL_QCEF is conceptually like the BNRHF but with different details.

(edit:spelling)

hkershaw-brown commented 3 months ago

note probably 2weeks ~+ before jla/Ian get to this to finish up.

todo notes for then

iangrooms commented 3 months ago

@hkershaw-brown is there anything outstanding before this can be merged?

I spoke with @jlaucar yesterday. I decided to rename from KERNEL_QCEF to KDE_FILTER. It matches the name of the distribution module as well as the distribution used in the probit transform, and it will hopefully clear up confusion stemming from having two filters with KERNEL in the name.

Regarding the rootfinding module and the new inv_cdf method that it provides: The main reason for using it with the new kernel density estimate (kde) methods is that it uses fewer evaluations of the cdf per step as compared to the existing inv_cdf. Evaluating the kde cdf can be quite a bit more expensive than evaluating the cdf for other distributions.

iangrooms commented 3 months ago

I do think it would help to link to the paper in the release notes, and 'Contributed by Ian Grooms' is plenty of attribution for me. Thanks very much for your help with this PR!