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] Owpaintdata, owpivot: ensure unique domain #4578

Merged
merged 6 commits into from
Apr 19, 2020

Conversation

AndrejaKovacic
Copy link
Contributor

Issue

Partialy fixes #4382

Includes
  • Code changes
  • Tests

Copy link
Contributor

@janezd janezd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wrote a few minor comments about Paint data.

As for the Pivot, I may be looking at a wrong thing but it seems to me that attributes are created in _create_group_tables and __get_group_table, where they are combined with attributes that come from the code that you modified. If I am not mistaken, I think that you should rename attributes in __get_group_table.

@@ -752,6 +753,7 @@ class Outputs:
table_name = Setting("Painted data")
attr1 = Setting("x")
attr2 = Setting("y")
unique_names = [attr1, attr2]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose there is no reason to store unique_names? Even if there is, this here won't work: unique_names will contain two instances of Setting. attr1 and attr2 are changed to "x" and "y" only in the meta class, after the class' namespace is filled.

Orange/widgets/data/owpaintdata.py Outdated Show resolved Hide resolved
Orange/widgets/data/owpaintdata.py Outdated Show resolved Hide resolved
@janezd janezd assigned AndrejaKovacic and unassigned janezd Apr 9, 2020
@AndrejaKovacic
Copy link
Contributor Author

I think i forgot to rename some of them in __get_group_table, but renaming them in create_pivot_domain is also necessary, since some of the domains are created there. Note, that domains created there are not necessarily part of the output but are there to be drawn in the widget. Renaming the variables in multiple places can cause some inconsistencies though.

@janezd janezd assigned janezd and unassigned AndrejaKovacic Apr 17, 2020
@janezd janezd merged commit 57de1b5 into biolab:master Apr 19, 2020
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.

Domain: Prevent constructing domain with non-unique variable names
2 participants