Skip to content
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

OWTabletoTimeseries: Accept meta attributes #61

Merged
merged 2 commits into from Jan 28, 2019

Conversation

Projects
None yet
3 participants
@ajdapretnar
Copy link
Collaborator

commented Nov 7, 2018

Issue

Fixes #51.

Description of changes

Accept meta attributes in As Timeseries widget.

Includes
  • Code changes
  • Tests
  • Documentation
if var.is_continuous and not isinstance(var, TimeVariable)]
if var.is_continuous and not isinstance(var,
TimeVariable)] + \
[var for var in data.domain.metas if isinstance(var, TimeVariable)]

This comment has been minimized.

Copy link
@kernc

kernc Nov 7, 2018

Member

Only accepts TimeVariables from metas? The two lines above list also other continuous variables. Like this:

Suggested change
[var for var in data.domain.metas if isinstance(var, TimeVariable)]
[var for var in data.domain.metas if var.is_time] + \
[var for var in data.domain.metas if var.is_continuous]

This comment has been minimized.

Copy link
@ajdapretnar

ajdapretnar Nov 7, 2018

Author Collaborator

Yes, I wasn't sure whether to include numeric or not. Easy fix.

This comment has been minimized.

Copy link
@ajdapretnar

ajdapretnar Nov 7, 2018

Author Collaborator

Can I ask why does the first two checks use isinstance? [var for var in data.domain.variables if isinstance(var, TimeVariable)]
Also, would it be nicer to:
[var for var in data.domain.metas if var.is_time or var.is_continuous]?
To avoid one extra loop?

This comment has been minimized.

Copy link
@kernc

kernc Jan 28, 2019

Member

The reason for the extra loop was maybe that continuous and time variables become grouped. I'm actually 👍 for that.

As Timeseries should accept attributes from X and metas.
"""
w = self.widget
data = Table("philadelphia-crime.csv.xz")[:20]

This comment has been minimized.

Copy link
@ajdapretnar

ajdapretnar Nov 7, 2018

Author Collaborator

Dude, I was literally testing with this same data set this afternoon. Where did it go? 😮 And what is a good simple alternative dataset to test with?

@ajdapretnar ajdapretnar force-pushed the ajdapretnar:astimeseries-metas branch from 6d2baed to ebeb8c2 Jan 28, 2019

@ajdapretnar

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 28, 2019

@kernc Fixed.

@codecov-io

This comment has been minimized.

Copy link

commented Jan 28, 2019

Codecov Report

Merging #61 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #61   +/-   ##
=======================================
  Coverage   67.06%   67.06%           
=======================================
  Files           7        7           
  Lines         671      671           
  Branches      102      102           
=======================================
  Hits          450      450           
  Misses        175      175           
  Partials       46       46

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b987e1a...ec3c15e. Read the comment docs.

@kernc

kernc approved these changes Jan 28, 2019

@@ -66,9 +66,10 @@ def set_data(self, data):
if data.domain.has_continuous_attributes():
vars = [var for var in data.domain.variables if isinstance(var, TimeVariable)] + \
[var for var in data.domain.variables
if var.is_continuous and not isinstance(var, TimeVariable)]
if var.is_continuous and not isinstance(var,
TimeVariable)] + \

This comment has been minimized.

Copy link
@kernc

kernc Jan 28, 2019

Member

and not var.is_time? Can also var.is_time on the instance check above.

This comment has been minimized.

Copy link
@ajdapretnar

ajdapretnar Jan 28, 2019

Author Collaborator

If TimeVariable is a ContinuousVariable, could we not simply

vars = [var for var in data.domain.variables if var.is_continuous] + \
                   [var for var in data.domain.metas if var.is_continuous]

This comment has been minimized.

Copy link
@kernc

kernc Jan 28, 2019

Member

Yes. This was to make variables appear grouped.

This comment has been minimized.

Copy link
@ajdapretnar

ajdapretnar Jan 28, 2019

Author Collaborator

Ah, right. I'll stick to that. I think it is nice, having them grouped.

if var.is_continuous and not isinstance(var, TimeVariable)]
if var.is_continuous and not isinstance(var,
TimeVariable)] + \
[var for var in data.domain.metas if var.is_time or var.is_continuous]

This comment has been minimized.

Copy link
@kernc

kernc Jan 28, 2019

Member

TimeVariable is a ContinuousVariable, so var.is_continuous check should be enough.

@ajdapretnar ajdapretnar force-pushed the ajdapretnar:astimeseries-metas branch from ebeb8c2 to ec3c15e Jan 28, 2019

@kernc kernc merged commit dc49b4d into biolab:master Jan 28, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.