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.

Sign up for GitHub

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

Jump to bottom

[FIX] Unique domain checks #4760

Merged
merged 14 commits into from
May 21, 2020
Merged

[FIX] Unique domain checks #4760

merged 14 commits into from
May 21, 2020

Conversation

AndrejaKovacic
Copy link
Contributor

@AndrejaKovacic AndrejaKovacic commented May 12, 2020 •
edited
Loading

Issue

Fixes #4382

Includes
  • Code changes
  • Tests

@AndrejaKovacic AndrejaKovacic mentioned this pull request May 12, 2020
Closed
@codecov
Copy link

codecov bot commented May 12, 2020 •
edited
Loading

Codecov Report

Merging #4760 into master will increase coverage by 0.01%.
The diff coverage is 98.92%.

@@            Coverage Diff             @@
##           master    #4760      +/-   ##
==========================================
+ Coverage   83.86%   83.87%   +0.01%     
==========================================
  Files         281      276       -5     
  Lines       56804    56057     -747     
==========================================
- Hits        47638    47020     -618     
+ Misses       9166     9037     -129

@AndrejaKovacic AndrejaKovacic changed the title [FIX] Unique domain checks visualization widgets [FIX] Unique domain checks visualization, evaluation widgets May 12, 2020
@PrimozGodec
Copy link
Contributor

Tests started to fail due to the new scikit release. Hope this PR fixes it #4768

@AndrejaKovacic AndrejaKovacic changed the title [FIX] Unique domain checks visualization, evaluation widgets [FIX] Unique domain checks May 13, 2020
@janezd janezd self-assigned this May 15, 2020
janezd
janezd requested changes May 15, 2020
View reviewed changes
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 have a few comments that you can ignore, and one that needs checking, in OWPCA.

In the future: commits should have (more) self contained names. Preferrably start with a widget or module name, then write what you changed, like the later commits in this PR.

Orange/evaluation/testing.py Outdated Show resolved Hide resolved
Orange/widgets/visualize/owvenndiagram.py Show resolved Hide resolved
Orange/widgets/evaluate/owpredictions.py Outdated
@@ -545,8 +545,7 @@ def _commit_predictions(self):

attrs = list(self.data.domain.attributes)
metas = list(self.data.domain.metas)

names = [var.name for var in chain(attrs, [self.class_var], metas)]
names = [var.name for var in chain(attrs, [self.class_var], metas) if var]
Copy link
Contributor

Choose a reason for hiding this comment

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

Why didn't you just use domain.class_vars, like in silhouette?
(Change if you wish, ignore if you don't.)

Orange/widgets/unsupervised/owpca.py Outdated Show resolved Hide resolved
Orange/widgets/unsupervised/owpca.py Outdated Show resolved Hide resolved
Orange/widgets/unsupervised/tests/test_owpca.py Outdated Show resolved Hide resolved
@AndrejaKovacic
Copy link
Contributor Author

Regarding Feature Constructor: variables are renamed one by one, immediately when created. Change of name is visible in gui immediately.
Renaming all the vars would result in issues with references in compute value for newly created vars. Renaming only new ones with get_unique_names has a problem if new names are not unique.

janezd
janezd reviewed May 17, 2020
View reviewed changes
Orange/widgets/unsupervised/owpca.py Outdated
dom = Domain(
[ContinuousVariable(a.name, compute_value=lambda _: None)
for a in self._pca.orig_domain.attributes],
metas=[StringVariable(name=meta_var)])
metas=[StringVariable(name='component')])
Copy link
Contributor

Choose a reason for hiding this comment

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

I fear you reverted too much.

Outputs "Data" and "Transformed data" don't need checking (as far as I see) but the third output, Components, still does. If apply PCA to data that contains a variable named "component", I get a table with two variables named "component".

@janezd janezd merged commit 42fc2cd into biolab:master May 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@janezd janezd janezd approved these changes

Assignees

@janezd janezd

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Domain: Prevent constructing domain with non-unique variable names
3 participants
@AndrejaKovacic @PrimozGodec @janezd