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

[FIX] Timeseries: time_variable now created in Timeseries #33

Merged
merged 2 commits into from Jan 8, 2018

Conversation

Projects
None yet
3 participants
@jerneju
Copy link
Contributor

commented Dec 15, 2017

Issue

Some widgets (Seasonal Adjustment, Moving Transform, and Difference) create Timeseries when they get Table and they do not set time_variable.

Description of changes

Scripting part which creates time_variable is now moved from widget As Timeseries to Timeseries.
Now this code can be used when creating Timeseries from Table.

Includes
  • Code changes
  • Tests
  • Documentation
pass
try:
self.time_variable = next(var for var in self.domain.attributes
if isinstance(var, ContinuousVariable))

This comment has been minimized.

Copy link
@kernc

kernc Dec 15, 2017

Member

I don't think any ContinuousVariable will cut it.

@kernc

This comment has been minimized.

Copy link
Member

commented Dec 15, 2017

I don't like this. Where does it happen?

@jerneju jerneju changed the title [FIX] Timeseries: do not create object if there is no TimeVariable [WIP][FIX] Timeseries: do not create object if there is no TimeVariable Dec 18, 2017

@jerneju jerneju force-pushed the jerneju:timeseries-timevar branch 2 times, most recently from 1b4b58c to a547ddd Dec 20, 2017

@codecov-io

This comment has been minimized.

Copy link

commented Dec 20, 2017

Codecov Report

Merging #33 into master will decrease coverage by 0.42%.
The diff coverage is 14.28%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #33      +/-   ##
==========================================
- Coverage   55.88%   55.45%   -0.43%     
==========================================
  Files           7        7              
  Lines         646      651       +5     
  Branches       99      101       +2     
==========================================
  Hits          361      361              
- Misses        249      253       +4     
- Partials       36       37       +1
Impacted Files Coverage Δ
orangecontrib/timeseries/timeseries.py 75.8% <0%> (-6.65%) ⬇️
orangecontrib/timeseries/functions.py 61.18% <50%> (ø) ⬆️

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 f3f4248...fa14b63. Read the comment docs.

@jerneju jerneju changed the title [WIP][FIX] Timeseries: do not create object if there is no TimeVariable [WIP][FIX] Timeseries: time_variable Dec 20, 2017

@jerneju jerneju force-pushed the jerneju:timeseries-timevar branch from a547ddd to 5ea28c4 Dec 20, 2017

@jerneju jerneju changed the title [WIP][FIX] Timeseries: time_variable [FIX] Timeseries: time_variable Dec 20, 2017

@jerneju jerneju changed the title [FIX] Timeseries: time_variable [FIX] Timeseries: time_variable created in Timeseries Dec 20, 2017

@jerneju jerneju changed the title [FIX] Timeseries: time_variable created in Timeseries [FIX] Timeseries: time_variable now created in Timeseries Dec 20, 2017

@jerneju jerneju force-pushed the jerneju:timeseries-timevar branch from 5ea28c4 to e9f0d96 Dec 20, 2017

@jerneju

This comment has been minimized.

Copy link
Contributor Author

commented Dec 20, 2017

@jerneju

This comment has been minimized.

Copy link
Contributor Author

commented Dec 20, 2017

@kernc

This comment has been minimized.

Copy link
Member

commented Jan 3, 2018

I don't like this change. .time_variable can be None. The failure happens on https://github.com/biolab/orange3-timeseries/pull/33/files#diff-baa207cfea1077bb261faf9efcf809c8R93. How about if the assertion assert var in self.domain was changed to:

if var is None:
    self.attributes = self.attributes.copy()
    self.attributes.pop('time_variable')
    return

assert var in self.domain

Would that work?

@jerneju

This comment has been minimized.

Copy link
Contributor Author

commented Jan 4, 2018

@kernc : And you also do not want to have a conversion from Table to Timeseries in a scripting part?

@kernc

This comment has been minimized.

Copy link
Member

commented Jan 4, 2018

Why, doesn't Timeseries.from_data_table(table) do that?

@jerneju jerneju force-pushed the jerneju:timeseries-timevar branch from e9f0d96 to 8e2d0e2 Jan 8, 2018

@jerneju jerneju force-pushed the jerneju:timeseries-timevar branch from 8e2d0e2 to fa14b63 Jan 8, 2018

@kernc

This comment has been minimized.

Copy link
Member

commented Jan 8, 2018

This looks safer to me. Thanks.

@kernc kernc merged commit cc106be into biolab:master Jan 8, 2018

1 check passed

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

@jerneju jerneju deleted the jerneju:timeseries-timevar branch Jan 9, 2018

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.