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

[WIP] [FIX] OWMergeData removes table meta attributes #3336

Closed
wants to merge 6 commits into from

Conversation

AndrejaKovacic
Copy link
Contributor

Issue

Fixes #3289

Description of changes

I added an optional parameter in table.py, method from_numpy can now accept data_attributes.

Includes
  • Code changes
  • Tests
  • Documentation

@CLAassistant
Copy link

CLAassistant commented Oct 26, 2018

CLA assistant check
All committers have signed the CLA.

@lanzagar lanzagar added this to the 3.18 milestone Oct 26, 2018
@lanzagar lanzagar modified the milestones: 3.18, 3.19 Nov 13, 2018
@thocevar thocevar self-assigned this Nov 23, 2018
@janezd
Copy link
Contributor

janezd commented Nov 23, 2018

Could you also add the argument data_attributes to Table.from_list, for the sake of consistency? Alternatively, you can remove the argument from from_numpy and just attach attributes in the widget (instead of passing them as an argument), that is, without modifying Orange.data.table. I have no preference.

Please remove the unwanted unrelated change in application.py. :)

Also, please squash the commits into a single one.

@janezd janezd assigned janezd and unassigned thocevar Nov 30, 2018
@lanzagar
Copy link
Contributor

Well, in addition to from_list there are from_domain, from_table, from_table_rows above and none have this parameter. So my preference is to leave the Table API unchanged and fix the widget instead.
And looking at how the tables are merged, I would say copying the attributes is not the only thing missing. At least the fact that ids are not retained is also a problem. Using from_numpy has been problematic in other cases (many of them now fixed) and is not a good choice here as well. Appending columns from extra_data to some primary data set should probably retain as much as possible from the main dataset (including ids and attributes).

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

Replaced by #3474.

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.

5 participants