Skip to content

[FIX] Mosaic Vizrank: compute_attr_order is called every step#2484

Merged
lanzagar merged 1 commit into
biolab:masterfrom
jerneju:mosaic-rank-compute-relief
Aug 3, 2017
Merged

[FIX] Mosaic Vizrank: compute_attr_order is called every step#2484
lanzagar merged 1 commit into
biolab:masterfrom
jerneju:mosaic-rank-compute-relief

Conversation

@jerneju
Copy link
Copy Markdown
Contributor

@jerneju jerneju commented Jul 21, 2017

Issue

compute_attr_order is called every step. It does not need to be.

Description of changes
Includes
  • Code changes
  • Tests
  • Documentation

@jerneju jerneju changed the title [FIX] Mosaic Vizrank: attr_order is calculated every step [FIX] Mosaic Vizrank: compute_attr_order is called every step Jul 21, 2017
@codecov-io
Copy link
Copy Markdown

codecov-io commented Jul 21, 2017

Codecov Report

Merging #2484 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2484      +/-   ##
==========================================
+ Coverage   74.56%   74.59%   +0.02%     
==========================================
  Files         320      320              
  Lines       56207    56209       +2     
==========================================
+ Hits        41913    41930      +17     
+ Misses      14294    14279      -15

Return the number of combinations, starting with a single attribute
if Mosaic is colored by class distributions, and two if by Pearson
"""
self.compute_attr_order()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I agree it was not very good to compute the order every time one needed the count.
But that line was there because of the next one, which sets n_attrs using attr_ordering. Removing just the first and still relying on len(self.attr_ordering) is not the best.
Can you change the next line to actually compute the number of attributes? Should be trivial using len of the domain, just be careful to (not) count the class appropriately.
Then the extra calls in tests will not be needed as well.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed. I also removed some dead code. It has not been used since changing color (class) was introduced.

@lanzagar
Copy link
Copy Markdown
Contributor

Seems there are still some issues.
Try iris and move the class to metas. Then run vizrank.

primitive meta data now used as attributes
updated test
@lanzagar
Copy link
Copy Markdown
Contributor

lanzagar commented Aug 3, 2017

There are issues with subset data, but unrelated with the changes and should be fixed in another PR.

@lanzagar lanzagar merged commit 3a5d0d4 into biolab:master Aug 3, 2017
@jerneju jerneju deleted the mosaic-rank-compute-relief branch August 7, 2017 07:49
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.

3 participants