Conversation
7446005 to
770f21d
Compare
ext/ensemble_builder.jl
Outdated
| pkgversion(ClimaAnalysis) > v"0.5.19" || error( | ||
| "Using GEnsembleBuilder requires a version of ClimaAnalysis above 0.5.19", |
There was a problem hiding this comment.
I don't know if we want to continue supporting ClimaAnalysis v0.5.19 and below, since there will probably need to be a change to support passing flatten keyword arguments when constructing the covariance matrix.
There was a problem hiding this comment.
I think we should only support >0.5.19, I don't think we have users that want to use old versions of ClimaAnalysis with this package anyway.
| function _is_compatible_with_metadata(var::OutputVar, metadata::Metadata) | ||
| return _same_short_names(var, metadata) && | ||
| _same_dim_names(var, metadata) && | ||
| _same_dim_units(var, metadata) && | ||
| _same_units(var, metadata) && | ||
| # For the temporal dimension, only check if the times of metadata is | ||
| # a subset of the times of var, because _match_dates will get the | ||
| # correct dates for us | ||
| _compatible_dims_values(var, metadata) | ||
| end |
There was a problem hiding this comment.
This need to be changed, since it is not very helpful to the user right now. For example, if a OutputVar doesn't match with any of the metadata, it would help if there was logging that tell you why it can't match with the metadata that was tried. Instead, it tells you that none of the metadata the function tried match with the OutputVar. This could be resolved with a verbose keyword argument or another function that can be used to diagnose the issues. Furthermore, I would like this system to be extensible, where the user can pass in their own custom checks.
I was thinking of doing it in another PR to avoid this one to become more bloated than it is right now.
There was a problem hiding this comment.
I agree, this will be an important addition in a future PR.
|
I think this is good enough for a review, but not ready to merge (still need to release a ClimaAnalysis version and missing/helpful features for building G ensemble matrix). |
| !!! info "Spinup and windowing times" | ||
| Internally, the correct dates are matched between the observational and | ||
| simulation data. As a result, you do not need to window the times (e.g. when | ||
| removing spinup) to match the times of the observations. |
There was a problem hiding this comment.
I am not too sure about this, since it could introduce correctness issues that might be overlooked. Although, it is convenient to automatically just match times, so you don't need to worry about windowing or slicing the time dimension.
ext/ensemble_builder.jl
Outdated
| ::Type{FT}, | ||
| ekp::EKP.EnsembleKalmanProcess, | ||
| ) where {FT <: AbstractFloat} | ||
| pkgversion(ClimaAnalysis) > v"0.5.19" || error( |
There was a problem hiding this comment.
Make sure to add a check for EKP as well
There was a problem hiding this comment.
The plan is to use the latest version of EnsembleKalmanProcesses and ClimaAnalysis, so this is not necessary.
nefrathenrici
left a comment
There was a problem hiding this comment.
This looks great so far! I think we can push this a bit further and make more information easily available to the user.
ext/ensemble_builder.jl
Outdated
| pkgversion(ClimaAnalysis) > v"0.5.19" || error( | ||
| "Using GEnsembleBuilder requires a version of ClimaAnalysis above 0.5.19", |
There was a problem hiding this comment.
I think we should only support >0.5.19, I don't think we have users that want to use old versions of ClimaAnalysis with this package anyway.
|
The |
48d73fb to
3a44f20
Compare
|
I think this is ready for another review. The only change that I want to make after this is maybe looking over how the functions are named and making another function in ClimaAnalysis to replace most of |
nefrathenrici
left a comment
There was a problem hiding this comment.
I think this is ready to merge, I just left some small comments that we can discuss later today.
| allunique(ClimaAnalysis.dates(var)) || @warn( | ||
| "Dates in OutputVar with short name $(short_name(var)) are not unique. You will not be able to use GEnsembleBuilder", | ||
| ) | ||
| end | ||
|
|
||
| any(==(""), ClimaAnalysis.short_name.(vars)) && @warn( |
There was a problem hiding this comment.
Should these just be warnings or actual errors?
There was a problem hiding this comment.
We discussed this offline, but these should be warnings because an user might not want to use GEnsembleBuilder for whatever reason.
| function _is_compatible_with_metadata(var::OutputVar, metadata::Metadata) | ||
| return _same_short_names(var, metadata) && | ||
| _same_dim_names(var, metadata) && | ||
| _same_dim_units(var, metadata) && | ||
| _same_units(var, metadata) && | ||
| # For the temporal dimension, only check if the times of metadata is | ||
| # a subset of the times of var, because _match_dates will get the | ||
| # correct dates for us | ||
| _compatible_dims_values(var, metadata) | ||
| end |
There was a problem hiding this comment.
I agree, this will be an important addition in a future PR.
| ClimaCalibrate.EnsembleBuilder.is_complete | ||
| ClimaCalibrate.EnsembleBuilder.get_g_ensemble | ||
| ClimaCalibrate.EnsembleBuilder.ranges_by_short_name |
There was a problem hiding this comment.
For is_complete, there should be a variant of this for checking a single column. This is helpful since if it is not completed for the first column, it probably won't be completed for any of the columns.
cfc0329 to
6ecc0b9
Compare
3de3183 to
a3f6824
Compare
e797c55 to
741d576
Compare
The function used to be private, but with the release of v0.5.20, the function is public and should be used instead.
Checks for unique dates and short name are added to ensure that `GEnsembleBuilder` with the observations.
dd617d6 to
d980e59
Compare
e880a55 to
8446d54
Compare
8446d54 to
f9ac127
Compare
closes #221, closes #211, closes #220
This PR adds
GEnsembleBuilderto facilitate easy construction of the G ensemble matrix using the metadata in the observation.