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] Detect duplicate names of variables in projections. #4550

Merged
merged 1 commit into from
Mar 24, 2020

Conversation

NejcDebevec
Copy link
Contributor

@NejcDebevec NejcDebevec commented Mar 19, 2020

Issue

Fixes #4497

Description of changes

Check for duplicates in original domain, not only in the selected one.

Includes
  • Code changes
  • Tests
  • Documentation

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Copy link
Contributor

@VesnaT VesnaT left a comment

Choose a reason for hiding this comment

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

Also, adding a test would be nice.

@@ -145,7 +145,7 @@ def proj_variable(i, name):
def _get_var_names(self, n):
postfixes = ["x", "y"] if n == 2 else [str(i) for i in range(1, n + 1)]
names = [f"{self.var_prefix}-{postfix}" for postfix in postfixes]
return get_unique_names(self.orig_domain, names)
return names
Copy link
Contributor

Choose a reason for hiding this comment

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

Since DomainProjection can be used independently of any widget, I'd leave it as it was (retain the get_unique_names)

self.projection.domain.attributes])

attributes = tuple([a.copy(name=n) for n, a in
zip(names, self.projection.domain.attributes)])
Copy link
Contributor

Choose a reason for hiding this comment

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

Lint is complaining about the indentation. Speaking of lint, please keep the line length up to 79 characters.

@@ -704,10 +704,17 @@ def _manual_move_finish(self, anchor_idx, x, y):
def _get_projection_data(self):
if self.data is None or self.projection is None:
return None

names = get_unique_names(self.data.domain, [a.name for a in
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe the variables should be copied only when necessary, i.e. when the obtained names are different than the proposed.

@codecov
Copy link

codecov bot commented Mar 23, 2020

Codecov Report

Merging #4550 into master will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #4550      +/-   ##
==========================================
- Coverage   83.55%   83.54%   -0.02%     
==========================================
  Files         281      276       -5     
  Lines       56126    55326     -800     
==========================================
- Hits        46895    46221     -674     
+ Misses       9231     9105     -126     

@NejcDebevec NejcDebevec force-pushed the unique_names branch 4 times, most recently from ad35968 to 7eb23af Compare March 24, 2020 07:53
Apply the requested changes.

Fix pylint error

Add code

Fix projection data domain duplicate checking.

Add test for unique name after linearprojection components added.

Refactor code.
@VesnaT VesnaT merged commit 8815160 into biolab:master Mar 24, 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.

Linear Projection: Unique variable names isn't working properly.
3 participants