CLIMATE-657 - Adding functions to calculate metrics #221
Conversation
huikyole
commented
Aug 12, 2015
huikyole
commented
- Remove metrics.SpatialMeanOfTemporalMeanBias
- Add metrics.SpatialPatternTaylorDiagram
- Add functions calc_bias, calc_stddev, calc_stddev_ratio, calc_correlation, calc_rmse
- Update test_metrics.py
- Remove metrics.SpatialMeanOfTemporalMeanBias - Add metrics.SpatialPatternTaylorDiagram - Add functions calc_bias, calc_stddev, calc_stddev_ratio, calc_correlation, calc_rmse - Update test_metrics.py
|
Merged build triggered. Test Failed. |
|
Merged build started. Test Failed. |
|
Merged build finished. Test Passed. |
|
Merged build triggered. Test Failed. |
|
Merged build started. Test Failed. |
|
Merged build finished. Test Passed. |
| np.testing.assert_array_equal(self.mean_bias.run(self.reference_dataset, self.target_dataset, True), expected_result) | ||
|
|
||
|
|
||
| class TestSpatialMeanOfTemporalMeanBias(unittest.TestCase): |
There was a problem hiding this comment.
Hey @huikyole, is there a reason why we're dropping this here? I'm not understanding why this is getting dropped from reading the JIRA and commit message but perhaps I'm overlooking something.
There was a problem hiding this comment.
@MJJoyce I removed the 'absolute=True/False' key word option from metrics.TemporalMeanBias. So we do not need this test. Users can always use numpy.absolute function to properly handle missing values after calculating biases.
There was a problem hiding this comment.
Hey @huikyole, so if we don't need the test then we don't need the metric that it's testing I gather? This sounds like something that should be under another ticket so it can be discussed outside of the scope of this ticket which seems to me to be changing the call structure of metrics so they use isolated functions that take arrays instead of OCW dataset objects. Thoughts?
There was a problem hiding this comment.
@MJJoyce, I agree with the point. However, I just could not leave unreasonable functions while updating metrics and test_metrics. It is too hard to manage many branches to separate all trivial updates while working to an approaching deadline. I never expect that anyone is using this function for any purpose. The ticket's title can be misleading. How do you think about revising the JIRA issue?
|
Truthfully @huikyole I don't see how managing this in a different branch is too complicated. The changes aren't dependent upon each other in any way so the branches don't even need to be merged together while you're doing your work right? And you're back to working on your old issue in just a few minutes. Worst case is there's some minor merge conflict here that takes all of 5 seconds to fix when you go to merge the PR. Concerns are easily separated then and we're not burying changes in unrelated tickets. There's also no reason to just remove the test and not the relevant metric, and that's big enough of a change to be in its own ticket I would think. |
|
Merged build triggered. Test Failed. |
|
Merged build started. Test Failed. |
|
Merged build finished. Test Passed. |
