-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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] OWMergeData removes table meta attributes #3474
Conversation
Is this changing the input data? The input data should never be modified (if any other widgets were also using the same data they would break). To support workflows, we should never modify data in place. |
What @markotoplak said. And although that could be fixed by making a copy of the data first and modifying that, I don't think you should just overwrite the domain of a Table, the arrays X, Y, metas, etc. It would be too easy to end up with an inconsistent Table (the shape of X might not agree with the size of the domain, ids, ...). |
I think the most appropriate solution would be to use the function from_numpy and add an option to add metadata as a parameter. I know @lanzagar disagrees with it, because there are some issues with that function, but maybe we should fix from_numpy, instead of trying to create new table here. |
About travis errors: lint issues are obvious, while to avoid segfault in documentation, you need to rebase to today's master. |
Orange/widgets/data/owmergedata.py
Outdated
table.name = getattr(self.data, 'name') | ||
table.attributes = getattr(self.data, 'attributes') | ||
table.ids = getattr(self.data, 'ids') | ||
return table |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know this code well, but judging by what I see in the Table
class, I think that you should have getattr(self.data, 'name', '')
and getattr(self.data, 'attributes', {})
.
ids
seems to be always present, so it's just table.ids = self.data.ids
.
(Perhaps name
and attributes
are always present, too, but a lot of code does getattr
for them.)
Codecov Report
@@ Coverage Diff @@
## master #3474 +/- ##
==========================================
+ Coverage 83.5% 83.55% +0.05%
==========================================
Files 367 367
Lines 65182 65466 +284
==========================================
+ Hits 54429 54703 +274
- Misses 10753 10763 +10 |
Codecov Report
@@ Coverage Diff @@
## master #3474 +/- ##
==========================================
+ Coverage 83.57% 83.57% +<.01%
==========================================
Files 367 367
Lines 65536 65548 +12
==========================================
+ Hits 54769 54782 +13
+ Misses 10767 10766 -1 |
3bfac1b
to
13fd644
Compare
13fd644
to
383c2aa
Compare
Burning some midnight oil? |
I was going to coment it's not even midnight yet. Time flies :) |
Issue
Fixes #3289
Description of changes
Merged data is added to original table instead of creating a new one.
Includes