Conversation
ph-kev
force-pushed
the
kp/checks
branch
7 times, most recently
from
1dbbacd to
65d7001
Compare
|
This PR is not ready to be merged in, but I would like a review before working on it anymore. The only checker that should be provided, but is not in this PR is checking the signs between simulation data and observational data. I think that functionality though should belong in another PR. |
ph-kev
force-pushed
the
kp/checks
branch
3 times, most recently
from
4ebc739 to
858f1b6
Compare
nefrathenrici
left a comment
nefrathenrici
left a comment
There was a problem hiding this comment.
Thank you! I like that this is easily extensible.
ext/checkers.jl
Outdated
| obs_dates = ClimaAnalysis.dates(metadata) | ||
| sim_dates = ClimaAnalysis.dates(var) | ||
| for dates in (obs_dates, sim_dates) | ||
| allunique(dates) || error("Dates in $dates are not unique") |
There was a problem hiding this comment.
Should this throw an error? I think this could just warn and return false.
There was a problem hiding this comment.
I think it should throw an error, because there shouldn't be duplicate dates.
I can remove this though, since this is checked in _match_dates.
There was a problem hiding this comment.
I decided to remove it. I think it should be moved to another checker or enforced everywhere, since it does not make sense to have duplicate dates.
ph-kev
force-pushed
the
kp/checks
branch
3 times, most recently
from
e479a09 to
0309982
Compare
|
I added documentation since the last review. |
2 participants
This PR refactors the matching of OutputVar to metadata to another module and add more functionality such as a verbose option and other
Matcherthat would be helpful.TODO