New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BUG: datawc does not work on a time axis. #2007

Merged
merged 1 commit into from Jun 7, 2016

Conversation

Projects
None yet
3 participants
@danlipsa
Contributor

danlipsa commented Jun 3, 2016

No description provided.

@danlipsa danlipsa force-pushed the sample_hov branch from f6b81f3 to 8c91171 Jun 3, 2016

BUG: datawc does not work on a time axis.
This happened because datawc is converted to cdtime.reltime type.

@danlipsa danlipsa force-pushed the sample_hov branch from 8c91171 to d7581f3 Jun 3, 2016

@danlipsa

This comment has been minimized.

Contributor

danlipsa commented Jun 3, 2016

@danlipsa

This comment has been minimized.

Contributor

danlipsa commented Jun 3, 2016

@doutriaux1 @aashish24 Please review.

@danlipsa danlipsa changed the title from Sample hov to BUG: datawc does not work on a time axis. Jun 3, 2016

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Jun 3, 2016

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Jun 3, 2016

cdtime and cdtime object have a comparison operator (cmp)

@danlipsa

This comment has been minimized.

Contributor

danlipsa commented Jun 3, 2016

@aashish24 Can you overload the comparison operator to compare to float as well as to cdtime.reltime? Note also that cdtime.reltime is C code.

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Jun 6, 2016

@danlipsa you cannot have multiple definitions of operators since python is type agnostic but what you can do is something like here:

class B(object):
    def __init__(self, bar):
        self.bar = bar
    def __eq__(self, other):
        if isinstance(other, B):
            return self.bar == other.bar
        elif isinstance(other, A):
            return self.bar + 3 == other.foo
        else:
            return NotImplemented
    def __ne__(self, other):
        result = self.__eq__(other)
        if result is NotImplemented:
            return result
        return not result

I would define this operator in axis class since that is in python. Referenced from here:
http://jcalderone.livejournal.com/32837.html

The advantage might be that once defined in axis.py, the comparison will work everywhere and hence reduced code duplication.

@danlipsa

This comment has been minimized.

Contributor

danlipsa commented Jun 6, 2016

@aashish24 Thanks for the pattern to override operators. Still, the problem we have to solve is to compare gm.datawc_ (either a float or the type returned by 'cdtime.reltime') with 1.e20. So it seems we would have to override = for cdtime.reltime which is C code.

Not sure I would like to hide through an operator the fact that datawc can have different types. I would rather investigate why datawc needs that, and make it only be float. @doutriaux1 Do you know why datawc gets converted to cdtime.reltime?

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Jun 6, 2016

@danlipsa it gets converted to relative time to accommodate for plotting data with different time units.

For example 2016-06-06 is 157 days since 2016-01-01 but is 5 days since 2016-06-01

So if you plot two datasets with the time but different untis, we need to convert to some "same units" (and calendar) time format ot be able to align the times properly. Does that make sense?

@danlipsa

This comment has been minimized.

Contributor

danlipsa commented Jun 6, 2016

@doutriaux1 Thanks for the explanation.

@danlipsa

This comment has been minimized.

Contributor

danlipsa commented Jun 6, 2016

So if you plot two datasets with the time but different untis, we need to convert to some "same units" (and calendar) time format ot be able to align the times properly.

@doutriaux1 Do we have a test for this?

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Jun 6, 2016

you could refactor these:

doutriaux1@omar:[/git/uvcdat/Packages/vcs/Test]:[pattern_scaling_aspect]:[7957]> ll *datawc*
-rw-rw-r-- 1 doutriaux1 doutriaux1  904 Mar 21 08:09 test_boxfill_datawc_time.py
-rw-rw-r-- 1 doutriaux1 doutriaux1  932 Mar 21 08:09 test_isofill_datawc_time.py
-rw-rw-r-- 1 doutriaux1 doutriaux1  896 Mar 21 08:09 test_isoline_datawc_time.py
-rw-rw-r-- 1 doutriaux1 doutriaux1 1.2K Mar 21 08:09 test_scatter_datawc_time.py
-rw-rw-r-- 1 doutriaux1 doutriaux1  905 Mar 21 08:09 test_vector_datawc_time.py
-rw-rw-r-- 1 doutriaux1 doutriaux1 1.2K Mar 21 08:09 test_xvsy_datawc_time.py
-rw-rw-r-- 1 doutriaux1 doutriaux1  947 Mar 21 08:09 test_xyvsy_datawc_time.py
-rw-rw-r-- 1 doutriaux1 doutriaux1  933 Mar 21 08:09 test_yxvsx_datawc_time.py
@danlipsa

This comment has been minimized.

Contributor

danlipsa commented Jun 6, 2016

@doutriaux1 @aashish24 is OK with merging this as is. If you are OK as well, I'll merge it.

# datawc_ can be a float or a cdtime.reltime
# TODO: Investigate why datawc is converted to a cdtime.reltime
def getDataWcValue(v):
if (type(v) is type(cdtime.reltime(0, 'months since 1900'))):

This comment has been minimized.

@doutriaux1

doutriaux1 Jun 6, 2016

Member

@danlipsa why months since 1900, we should use the gm.datawc_units and gm.datawc_calendar

This comment has been minimized.

@danlipsa

danlipsa Jun 6, 2016

Contributor

@doutriaux1 This is a way to get the type, so the actual value does not matter. I got this from Canvas.py:3051. Is there a better way to get the type returned by cdtime.reltime?

This comment has been minimized.

@doutriaux1

doutriaux1 Jun 6, 2016

Member

what about component time?

a=cdtime.reltime(45,"months since 1945")
b=cdtime.comptime(2000)
gm = x.createboxfill()
gm.datawc_x1 = b
type(gm.datawc_x1) is type(a)
False

Is it ok to return v if it's a component time?

This comment has been minimized.

@danlipsa

danlipsa Jun 6, 2016

Contributor

@doutriaux1 What is comptime? I did not try it, so probably not. It does not work with rel time either - I did try that. Its not clear from the documentation that datawc_ accepts objects beyond float values. I added an issue for this and we can fix it and add tests.
#2009
The question is: should we do this for 2.6 or leave it for later? Note this PR solves the user's issue. @aashish24 ?

This comment has been minimized.

@doutriaux1

doutriaux1 Jun 6, 2016

Member

ok. A comptime is a componentTime. Basically a date. Whereas reltime is a relative time, i.e a delta since some reference date. You should take a look at: http://uvcdat.llnl.gov/documentation/cdms/cdms_3.html

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Jun 7, 2016

@doutriaux1 @danlipsa few things:

  1. What the zone of componentTime? GMT?
  2. One thing I found confusing is that in this case, user has entered float value and then we convert it into reltime (on the same variable). I think it would be better
    2 (a): For conversion to store in a separate variable/
    2 (b): Not modify the original value and type as it leads to bugs / confusion for new users.
@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Jun 7, 2016

@aashish24 there's no local in comptime, it's a good suggestion, we might want to add this.

  1. is a good suggestion, actually we should do it for all vcs elements, there's a lot of conversion under the hood that leads to oiginal user values being changed. example "dot" becomes a number, etc... Let's open a PR for this issue.
@danlipsa

This comment has been minimized.

Contributor

danlipsa commented Jun 7, 2016

@doutriaux1 @aashish24 Should we merge this? We can clean-up and fix additional related datawc/time axis bugs in subsequent PRs.

@doutriaux1 doutriaux1 merged commit 18f40b7 into master Jun 7, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@doutriaux1 doutriaux1 deleted the sample_hov branch Jun 7, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment