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] OWMergeData removes table meta attributes #3474

Merged
merged 5 commits into from
Dec 21, 2018

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

Orange/widgets/data/owmergedata.py Outdated Show resolved Hide resolved
@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