Skip to content

[FIX] OWMergeData removes table meta attributes#3474

Merged
janezd merged 5 commits intobiolab:masterfrom
AndrejaKovacic:fix_metas
Dec 21, 2018
Merged

[FIX] OWMergeData removes table meta attributes#3474
janezd merged 5 commits intobiolab:masterfrom
AndrejaKovacic:fix_metas

Conversation

@AndrejaKovacic
Copy link
Contributor

Issue

Fixes #3289

Description of changes

Merged data is added to original table instead of creating a new one.

Includes
  • Code changes
  • Tests
  • Documentation

@markotoplak
Copy link
Member

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.

@lanzagar
Copy link
Contributor

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, ...).

@lanzagar lanzagar changed the title [FIX] OWMergeData removes table meta attributes [WIP] [FIX] OWMergeData removes table meta attributes Dec 14, 2018
@AndrejaKovacic
Copy link
Contributor Author

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.

@janezd
Copy link
Contributor

janezd commented Dec 21, 2018

About travis errors: lint issues are obvious, while to avoid segfault in documentation, you need to rebase to today's master.

table.name = getattr(self.data, 'name')
table.attributes = getattr(self.data, 'attributes')
table.ids = getattr(self.data, 'ids')
return table
Copy link
Contributor

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
Copy link

codecov bot commented Dec 21, 2018

Codecov Report

Merging #3474 into master will increase coverage by 0.05%.
The diff coverage is 100%.

@@            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
Copy link

codecov bot commented Dec 21, 2018

Codecov Report

Merging #3474 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            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

@janezd
Copy link
Contributor

janezd commented Dec 21, 2018

Burning some midnight oil?

@janezd janezd changed the title [WIP] [FIX] OWMergeData removes table meta attributes [FIX] OWMergeData removes table meta attributes Dec 21, 2018
@janezd janezd merged commit 0f189a5 into biolab:master Dec 21, 2018
@AndrejaKovacic
Copy link
Contributor Author

I was going to coment it's not even midnight yet. Time flies :)

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants