You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@climate.apache.org by huikyole <gi...@git.apache.org> on 2015/08/12 03:48:02 UTC
[GitHub] climate pull request: CLIMATE-657 - Adding functions to calculate ...
GitHub user huikyole opened a pull request:
https://github.com/apache/climate/pull/221
CLIMATE-657 - Adding functions to calculate metrics
- Remove metrics.SpatialMeanOfTemporalMeanBias
- Add metrics.SpatialPatternTaylorDiagram
- Add functions calc_bias, calc_stddev, calc_stddev_ratio, calc_correlation, calc_rmse
- Update test_metrics.py
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/huikyole/climate CLIMATE-657
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/climate/pull/221.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #221
----
commit 892676e77b1bd599597ebaed20ab52233e06ce96
Author: huikyole <hu...@argo.jpl.nasa.gov>
Date: 2015-08-12T01:41:12Z
CLIMATE-657 - Adding functions to calculate metrics
- Remove metrics.SpatialMeanOfTemporalMeanBias
- Add metrics.SpatialPatternTaylorDiagram
- Add functions calc_bias, calc_stddev, calc_stddev_ratio, calc_correlation, calc_rmse
- Update test_metrics.py
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] climate pull request: CLIMATE-657 - Adding functions to calculate ...
Posted by OCWJenkins <gi...@git.apache.org>.
Github user OCWJenkins commented on the pull request:
https://github.com/apache/climate/pull/221#issuecomment-130204025
Merged build triggered. Test Failed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] climate pull request: CLIMATE-657 - Adding functions to calculate ...
Posted by OCWJenkins <gi...@git.apache.org>.
Github user OCWJenkins commented on the pull request:
https://github.com/apache/climate/pull/221#issuecomment-131185416
Merged build triggered. Test Failed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] climate pull request: CLIMATE-657 - Adding functions to calculate ...
Posted by OCWJenkins <gi...@git.apache.org>.
Github user OCWJenkins commented on the pull request:
https://github.com/apache/climate/pull/221#issuecomment-130204045
Merged build started. Test Failed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] climate pull request: CLIMATE-657 - Adding functions to calculate ...
Posted by OCWJenkins <gi...@git.apache.org>.
Github user OCWJenkins commented on the pull request:
https://github.com/apache/climate/pull/221#issuecomment-130133776
Merged build finished. Test Passed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] climate pull request: CLIMATE-657 - Adding functions to calculate ...
Posted by OCWJenkins <gi...@git.apache.org>.
Github user OCWJenkins commented on the pull request:
https://github.com/apache/climate/pull/221#issuecomment-130133632
Merged build triggered. Test Failed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] climate pull request: CLIMATE-657 - Adding functions to calculate ...
Posted by OCWJenkins <gi...@git.apache.org>.
Github user OCWJenkins commented on the pull request:
https://github.com/apache/climate/pull/221#issuecomment-131186121
Merged build started. Test Failed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] climate pull request: CLIMATE-657 - Adding functions to calculate ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/climate/pull/221
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] climate pull request: CLIMATE-657 - Adding functions to calculate ...
Posted by huikyole <gi...@git.apache.org>.
Github user huikyole commented on a diff in the pull request:
https://github.com/apache/climate/pull/221#discussion_r36885212
--- Diff: ocw/tests/test_metrics.py ---
@@ -208,41 +229,6 @@ def test_function_run(self):
expected_result.fill(-300)
np.testing.assert_array_equal(self.mean_bias.run(self.target_dataset,self.reference_dataset), expected_result)
- def test_function_run_abs(self):
- '''Test mean bias function between reference dataset and target dataset with abs as True.'''
- expected_result = np.zeros((5, 5), dtype=np.int)
- expected_result.fill(300)
- np.testing.assert_array_equal(self.mean_bias.run(self.reference_dataset, self.target_dataset, True), expected_result)
-
-
-class TestSpatialMeanOfTemporalMeanBias(unittest.TestCase):
--- End diff --
@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?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] climate pull request: CLIMATE-657 - Adding functions to calculate ...
Posted by huikyole <gi...@git.apache.org>.
Github user huikyole commented on a diff in the pull request:
https://github.com/apache/climate/pull/221#discussion_r36877753
--- Diff: ocw/tests/test_metrics.py ---
@@ -208,41 +229,6 @@ def test_function_run(self):
expected_result.fill(-300)
np.testing.assert_array_equal(self.mean_bias.run(self.target_dataset,self.reference_dataset), expected_result)
- def test_function_run_abs(self):
- '''Test mean bias function between reference dataset and target dataset with abs as True.'''
- expected_result = np.zeros((5, 5), dtype=np.int)
- expected_result.fill(300)
- np.testing.assert_array_equal(self.mean_bias.run(self.reference_dataset, self.target_dataset, True), expected_result)
-
-
-class TestSpatialMeanOfTemporalMeanBias(unittest.TestCase):
--- End diff --
@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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] climate pull request: CLIMATE-657 - Adding functions to calculate ...
Posted by MJJoyce <gi...@git.apache.org>.
Github user MJJoyce commented on the pull request:
https://github.com/apache/climate/pull/221#issuecomment-130410320
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?
```
# Notice the thing to fix, make new JIRA for it
git checkout master
git checkout -b CLIMATE-<new ticket id>
# make the quick changes and commit
git push github CLIMATE-<new ticket id>
# make PR
```
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] climate pull request: CLIMATE-657 - Adding functions to calculate ...
Posted by OCWJenkins <gi...@git.apache.org>.
Github user OCWJenkins commented on the pull request:
https://github.com/apache/climate/pull/221#issuecomment-131186268
Merged build finished. Test Passed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] climate pull request: CLIMATE-657 - Adding functions to calculate ...
Posted by OCWJenkins <gi...@git.apache.org>.
Github user OCWJenkins commented on the pull request:
https://github.com/apache/climate/pull/221#issuecomment-130133641
Merged build started. Test Failed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] climate pull request: CLIMATE-657 - Adding functions to calculate ...
Posted by MJJoyce <gi...@git.apache.org>.
Github user MJJoyce commented on a diff in the pull request:
https://github.com/apache/climate/pull/221#discussion_r36883786
--- Diff: ocw/tests/test_metrics.py ---
@@ -208,41 +229,6 @@ def test_function_run(self):
expected_result.fill(-300)
np.testing.assert_array_equal(self.mean_bias.run(self.target_dataset,self.reference_dataset), expected_result)
- def test_function_run_abs(self):
- '''Test mean bias function between reference dataset and target dataset with abs as True.'''
- expected_result = np.zeros((5, 5), dtype=np.int)
- expected_result.fill(300)
- np.testing.assert_array_equal(self.mean_bias.run(self.reference_dataset, self.target_dataset, True), expected_result)
-
-
-class TestSpatialMeanOfTemporalMeanBias(unittest.TestCase):
--- End diff --
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?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] climate pull request: CLIMATE-657 - Adding functions to calculate ...
Posted by OCWJenkins <gi...@git.apache.org>.
Github user OCWJenkins commented on the pull request:
https://github.com/apache/climate/pull/221#issuecomment-130204435
Merged build finished. Test Passed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] climate pull request: CLIMATE-657 - Adding functions to calculate ...
Posted by MJJoyce <gi...@git.apache.org>.
Github user MJJoyce commented on a diff in the pull request:
https://github.com/apache/climate/pull/221#discussion_r36877042
--- Diff: ocw/tests/test_metrics.py ---
@@ -208,41 +229,6 @@ def test_function_run(self):
expected_result.fill(-300)
np.testing.assert_array_equal(self.mean_bias.run(self.target_dataset,self.reference_dataset), expected_result)
- def test_function_run_abs(self):
- '''Test mean bias function between reference dataset and target dataset with abs as True.'''
- expected_result = np.zeros((5, 5), dtype=np.int)
- expected_result.fill(300)
- np.testing.assert_array_equal(self.mean_bias.run(self.reference_dataset, self.target_dataset, True), expected_result)
-
-
-class TestSpatialMeanOfTemporalMeanBias(unittest.TestCase):
--- End diff --
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---