You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@climate.apache.org by kwhitehall <gi...@git.apache.org> on 2015/05/11 20:21:58 UTC
[GitHub] climate pull request: CLIMATE-634 Fix bug related to month_start >...
GitHub user kwhitehall opened a pull request:
https://github.com/apache/climate/pull/196
CLIMATE-634 Fix bug related to month_start > month_end
- Fix offset to apply to DS.values in if-else statement
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/kwhitehall/climate CLIMATE-634
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/climate/pull/196.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 #196
----
commit eb2c4ca19eb4abcfe433fe8fd1898aed3a51fbb7
Author: Kim Whitehall <ki...@jpl.nasa.gov>
Date: 2015-05-11T18:22:18Z
CLIMATE-634 Fixed bug related to month_start > month_end
----
---
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-634 Fix bug related to month_start >...
Posted by OCWJenkins <gi...@git.apache.org>.
Github user OCWJenkins commented on the pull request:
https://github.com/apache/climate/pull/196#issuecomment-101346694
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-634 Fix bug related to month_start >...
Posted by OCWJenkins <gi...@git.apache.org>.
Github user OCWJenkins commented on the pull request:
https://github.com/apache/climate/pull/196#issuecomment-101007064
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-634 Fix bug related to month_start >...
Posted by OCWJenkins <gi...@git.apache.org>.
Github user OCWJenkins commented on the pull request:
https://github.com/apache/climate/pull/196#issuecomment-101006182
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-634 Fix bug related to month_start >...
Posted by MJJoyce <gi...@git.apache.org>.
Github user MJJoyce commented on the pull request:
https://github.com/apache/climate/pull/196#issuecomment-144744427
Can we update this PR and get it reviewed/merged or close it if it's not relevant folks? @kwhitehall?
---
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-634 Fix bug related to month_start >...
Posted by MJJoyce <gi...@git.apache.org>.
Github user MJJoyce commented on the pull request:
https://github.com/apache/climate/pull/196#issuecomment-101328087
@chrismattmann, the normal workflow on the project is to push changes for review via PR. I don't think it's a great idea to suggest people to just push their changes when we all stick with this workflow. From original core developer to new project contributor we've stuck with this workflow since just after we graduated to a TLP. It generates valuable feedback on code changes and it keeps people on the project up to date on changes. Merges are generally handled quite quickly unless they break tests or make large changes that are best discussed first.
If people would like to change our workflow on the project (an overview of which is [on the wiki](https://cwiki.apache.org/confluence/display/CLIMATE/Developer+Getting+Started+Guide)) then that should be brought up on the lists and discussed. Similarly, if the documentation isn't sufficient to explain our workflow then let's make sure we fix that (also a good thing to bring up on the dev list I think). I'll go ahead and make a thread on the dev list where we can discuss this since this keeps getting brought up.
---
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-634 Fix bug related to month_start >...
Posted by chrismattmann <gi...@git.apache.org>.
Github user chrismattmann commented on the pull request:
https://github.com/apache/climate/pull/196#issuecomment-101155972
thanks @huikyole looking forward to seeing it. Also feel free to simply push it - everything between all of you guys doesn't need to be reviewed. If it looks good, passes tests, etc., please commit. If you are thinking you want feedback and comments and so forth feel free to ask for them. I'd also like to see anything that's more architecture related discussed on dev@climate.apache.org. We have fairly zero discussion there - we should fix that. I'll raise a thread on dev@climate.a.o about this.
---
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-634 Fix bug related to month_start >...
Posted by kwhitehall <gi...@git.apache.org>.
Github user kwhitehall commented on the pull request:
https://github.com/apache/climate/pull/196#issuecomment-101124957
@huikyole thank you for your review. As the tix indicates, this is a bug. If one tried to use month_start=12 and month_end=2 for a dataset, the code in the if block tried to access the DS (Dataset) to apply the offset as oppose to DS.values and hence was throwing an error.
---
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-634 Fix bug related to month_start >...
Posted by OCWJenkins <gi...@git.apache.org>.
Github user OCWJenkins commented on the pull request:
https://github.com/apache/climate/pull/196#issuecomment-101346682
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-634 Fix bug related to month_start >...
Posted by huikyole <gi...@git.apache.org>.
Github user huikyole commented on the pull request:
https://github.com/apache/climate/pull/196#issuecomment-101128446
First of all, without reshaping, the second dimension cannot be a month dimension at all.
dataset.values.shape = nt, ny, nx
How do you think you can fix the bug by removing reshape function?
The bug is not an issue here. This module was written just for a demonstration purpose two years ago when the input dataset was not an OCW dataset object. I would rather rewrite this code.
---
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-634 Fix bug related to month_start >...
Posted by OCWJenkins <gi...@git.apache.org>.
Github user OCWJenkins commented on the pull request:
https://github.com/apache/climate/pull/196#issuecomment-101006245
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-634 Fix bug related to month_start >...
Posted by kwhitehall <gi...@git.apache.org>.
Github user kwhitehall commented on the pull request:
https://github.com/apache/climate/pull/196#issuecomment-101379241
@huikyole I'm not entirely sure I am following your slide on PR #197
---
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-634 Fix bug related to month_start >...
Posted by kwhitehall <gi...@git.apache.org>.
Github user kwhitehall closed the pull request at:
https://github.com/apache/climate/pull/196
---
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-634 Fix bug related to month_start >...
Posted by kwhitehall <gi...@git.apache.org>.
Github user kwhitehall commented on the pull request:
https://github.com/apache/climate/pull/196#issuecomment-101771065
@darth-pr From the latter comments on @huikyole PR#197 it would appear that the request is to deprecate this and a number of other functions with another function called temporal_subset. I haven't fully reviewed the code towards that end - of replacing the other functions which were recently mentioned. I had only reviewed it wrt to replacing this function, which seemed neither here nor there. THIS tix was indicating that RIGHT now if you try to use the function utils.calc_climatology_season for a season that is defined between years as Dec-Jan-Feb (DJF), it WILL throw an error because the code incorrectly attempts to subset a DS Object as opposed to the DS.values array. BUT if you used the same function for a season that did not fall between years, e.g. Jun-Jul-Aug. It would work. On this PR discussion then ensued about if the subsetting that occurs in the DJF case actually works, and I provided a case using a 3D dataset representing the DS.values variable and a 1D array represe
nting the DS.times variable to demonstrate that the existing method does work. That said, the current PR does not contain the update for the DS.time variable (this would be an addition to the utils.calc_climatology_season and should be on a different tix in my opinion, but it was an excellent suggestion by @huikyole and I decided to demonstrate in the example that without overhauling the code, we can get it done - but it is not currently a part of this PR, I will create a separate tix depending on how this ends). As for the slide on #197 I'm still trying to figure out what that is about. As far as I can tell, the stuff in the column under my Github Handle are related to the "unittest" to demonstrate the code extracts the correct portions of a 3D array (like in a DS.values array) and the corresponding 1D array (like in the times array). I also indicated the same example using PR#197 with OCW DS will work here.
@MJJoyce @darth-pr I hope that sheds some light on the connectivity between the two PRs from my perspective
---
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-634 Fix bug related to month_start >...
Posted by MJJoyce <gi...@git.apache.org>.
Github user MJJoyce commented on the pull request:
https://github.com/apache/climate/pull/196#issuecomment-101748088
Hey @darth-pr, truthfully I'm a bit lost on most of these interlinked PR conversations. It seems #197 is trying to explain something regarding this but I'm really not sure. Hopefully someone can explain it to me.
---
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-634 Fix bug related to month_start >...
Posted by MJJoyce <gi...@git.apache.org>.
Github user MJJoyce commented on the pull request:
https://github.com/apache/climate/pull/196#issuecomment-101372878
@kwhitehall For the unittest you're going to want to put that with the other unit tests for the core library (ocw/tests) and name it inline with the other ones. For this particular issue since it's a test for something in utils it should go in the test_utils.py file. In fact there might already be tests for this function that you could add to around here:
https://github.com/apache/climate/blob/master/ocw/tests/test_utils.py#L174
If you have any questions let me know and I'm happy to help.
---
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-634 Fix bug related to month_start >...
Posted by huikyole <gi...@git.apache.org>.
Github user huikyole commented on the pull request:
https://github.com/apache/climate/pull/196#issuecomment-101081960
These changes do not make this module useful. As I suggested previously, this module does not use datetime information included in the input OCW dataset at all. Let's assume that the input dataset is monthly two-dimensional fields between 2000 June and 2011 April. In this case, month_start =12 and month_end=2 output DJF averaged data?
---
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-634 Fix bug related to month_start >...
Posted by huikyole <gi...@git.apache.org>.
Github user huikyole commented on the pull request:
https://github.com/apache/climate/pull/196#issuecomment-101151369
@chrismattmann A better update was part of another pull request. I had to remove the changes to generate a pull request for each issue. I will provide a function to replace this.
---
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-634 Fix bug related to month_start >...
Posted by kwhitehall <gi...@git.apache.org>.
Github user kwhitehall commented on the pull request:
https://github.com/apache/climate/pull/196#issuecomment-101144654
The reshape function was moved out of the if-else statement.
The offset variable is used to shift the DS.values to "begin" @ that month
The month_index is used to determine the slices to make.
---
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-634 Fix bug related to month_start >...
Posted by kwhitehall <gi...@git.apache.org>.
Github user kwhitehall commented on the pull request:
https://github.com/apache/climate/pull/196#issuecomment-101349340
@chrismattmann I've added a unittest as suggested. (I can clean it up later if we decide to keep it The purpose is to illustrate the shape of the datasets and which aspects are being captured. I can provide an example of it being used but the example provided by @huikyole is basically it. In that example under @huikyole CLIMATE-634tix, if you use utils.calc_climatology_season you will see the original bug indicated in this tix, and you'll observe that it the functionality works (at least for JJA, and this change will allow it work for DJF).
@huikyole I had a look at PR#197, I see you are trying to capture the DS.times showing the timeslicing that occurred (which isn't functionality available now). I agree this is useful, and should be captured. I have included in this unittest capturing the timeseries. That said, a discussion is also needed in my opinion wrt to what happens when the seasonal mean captured as oppose to the time series of the seasons i.e. what does one put in DS.times when the season is averaged over the years e.g. do we use datetime(1,startmth,day), datetime(1,endmth,day)? See http://www.cgd.ucar.edu/cms/eaton/netcdf/CF-20010629.htm#climatology
---
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-634 Fix bug related to month_start >...
Posted by MJJoyce <gi...@git.apache.org>.
Github user MJJoyce commented on the pull request:
https://github.com/apache/climate/pull/196#issuecomment-101378903
Ah I see @kwhitehall. I would say it really doesn't hurt to have more tests honestly. Worst case we can remove them later if they don't help out, but that's just my thought.
---
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-634 Fix bug related to month_start >...
Posted by chrismattmann <gi...@git.apache.org>.
Github user chrismattmann commented on the pull request:
https://github.com/apache/climate/pull/196#issuecomment-101156144
BTW, no need to reply directly - it's late and what are we all doing up??! reply tomorrow! :+1: both of you! :)
---
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-634 Fix bug related to month_start >...
Posted by OCWJenkins <gi...@git.apache.org>.
Github user OCWJenkins commented on the pull request:
https://github.com/apache/climate/pull/196#issuecomment-101346906
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-634 Fix bug related to month_start >...
Posted by chrismattmann <gi...@git.apache.org>.
Github user chrismattmann commented on the pull request:
https://github.com/apache/climate/pull/196#issuecomment-101125147
perhaps it's best also to speak in terms of test cases and what the expectations are. If the code is intended to provide the functionality that @huikyole is talking about, then we should have a unit test verifying that. If not, I would say we need to solve this with documentation on expected use.
---
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-634 Fix bug related to month_start >...
Posted by huikyole <gi...@git.apache.org>.
Github user huikyole commented on the pull request:
https://github.com/apache/climate/pull/196#issuecomment-144755310
@MJJoyce , #647 addressed this 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-634 Fix bug related to month_start >...
Posted by chrismattmann <gi...@git.apache.org>.
Github user chrismattmann commented on the pull request:
https://github.com/apache/climate/pull/196#issuecomment-101156111
@kwhitehall @huikyole do we have examples of where this code is actually called by something in which tests break? If not, we should write some tests first, with the expected behavior, which will allow us to understand the best way to fix it I 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-634 Fix bug related to month_start >...
Posted by kwhitehall <gi...@git.apache.org>.
Github user kwhitehall commented on the pull request:
https://github.com/apache/climate/pull/196#issuecomment-101378372
@MJJoyce thanks for the advice! I'm not entirely sure it's for the code base in general, more so for explanation of what's going on here. But if you want it in the codebase after this stuff is resolved, I'll clean it up.
---
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-634 Fix bug related to month_start >...
Posted by chrismattmann <gi...@git.apache.org>.
Github user chrismattmann commented on the pull request:
https://github.com/apache/climate/pull/196#issuecomment-101141392
hey @huikyole can you provide a better update here? If so, please suggest, would be happy to review.
---
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-634 Fix bug related to month_start >...
Posted by darth-pr <gi...@git.apache.org>.
Github user darth-pr commented on the pull request:
https://github.com/apache/climate/pull/196#issuecomment-101736736
@MJJoyce @kwhitehall the way I read @huikyole comment originally is that we should deprecate or remove this function. Are we trying to fix something here that we shouldn't fix and instead have an alternative? Does PR #197 merely supersede the work being done here?
---
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.
---