Conversation
ph-kev
force-pushed
the
kp/lat-weights
branch
3 times, most recently
from
2168c29 to
69473ee
Compare
ph-kev
force-pushed
the
kp/lat-weights
branch
from
69473ee to
9a17926
Compare
nefrathenrici
left a comment
nefrathenrici
left a comment
There was a problem hiding this comment.
This looks good so far! Just a few comments.
| """ | ||
| _apply_lat_weights_to_samples!(stacked_sample_matrix, | ||
| vars::Iterable{OutputVar}, | ||
| start_date, | ||
| end_date, | ||
| min_cosd_lat) | ||
|
|
||
| Apply latitude weights to the matrix of samples. | ||
|
|
||
| The latitude weights applied is `1 / sqrt(max(cosd(lat), 0.1))` to each column | ||
| of the matrix. | ||
| """ |
There was a problem hiding this comment.
Could you update this docstring to match the function signature?
There was a problem hiding this comment.
I thought this docstring matches the function signature? The only difference is the Iterable{OutputVar}, but there is no easy way to specify that an iterable of OutputVars as a type.
There was a problem hiding this comment.
This matches the the behavior for SVDplusDCovariance but this function itself doesn't have a default min_cosd_lat, I think it should be 1 / sqrt(max(cosd(lat), min_cosd_lat))
|
|
||
| """The minimum cosine weight when using latitude weighting""" | ||
| min_cosd_lat::FT3 | ||
| end |
There was a problem hiding this comment.
Could we use a single FT type parameter for this struct?
There was a problem hiding this comment.
I can do this, but should I do it for all the structs in ObservationRecipe?
There was a problem hiding this comment.
I think that would be the best, we don't need separate FT parameters.
There was a problem hiding this comment.
This is a little difficult, since there are default keyword arguments whose float types can differ from what the user put in.
There was a problem hiding this comment.
Ok, then it's fine as-is, there isn't much benefit to enforcing one FT anyway.
ph-kev
force-pushed
the
kp/lat-weights
branch
from
9a17926 to
317fc16
Compare
| """ | ||
| _apply_lat_weights_to_samples!(stacked_sample_matrix, | ||
| vars::Iterable{OutputVar}, | ||
| start_date, | ||
| end_date, | ||
| min_cosd_lat) | ||
|
|
||
| Apply latitude weights to the matrix of samples. | ||
|
|
||
| The latitude weights applied is `1 / sqrt(max(cosd(lat), 0.1))` to each column | ||
| of the matrix. | ||
| """ |
There was a problem hiding this comment.
This matches the the behavior for SVDplusDCovariance but this function itself doesn't have a default min_cosd_lat, I think it should be 1 / sqrt(max(cosd(lat), min_cosd_lat))
|
|
||
| """The minimum cosine weight when using latitude weighting""" | ||
| min_cosd_lat::FT3 | ||
| end |
There was a problem hiding this comment.
I think that would be the best, we don't need separate FT parameters.
ph-kev
force-pushed
the
kp/lat-weights
branch
from
317fc16 to
b86d3d6
Compare
test/observation_recipe.jl
Outdated
| @test !isequal( | ||
| svd_plus_d_covar_with_lat_weights.svd_cov, | ||
| svd_plus_d_covar_with_no_lat_weights.svd_cov, | ||
| ) |
ph-kev
force-pushed
the
kp/lat-weights
branch
2 times, most recently
from
e801a6a to
8afea03
Compare
2 participants
closes #229, closes #199 - This PR adds latitude weighting to the
SVDplusDCovariancematrix.