[ENH] Pythagorean tree and Pythagorean forest widgets #1441
Conversation
Current coverage is 88.17%@@ master #1441 diff @@
==========================================
Files 77 77
Lines 7591 7613 +22
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
+ Hits 6688 6713 +25
+ Misses 903 900 -3
Partials 0 0
|
| if self.SIZE_CALCULATION[self.size_calc_idx][0] == 'Logarithmic': | ||
| self.log_scale_box.parent().setEnabled(True) | ||
| else: | ||
| self.log_scale_box.parent().setEnabled(False) |
There was a problem hiding this comment.
self.log_scale_box.parent().setEnabled(
self.SIZE_CALCULATION[self.size_calc_idx][0] == 'Logarithmic')
|
Please enable tests in Orange/widgets/visualize/widgetutils/tree/tests. |
|
Please do what you find reasonable to make pylint more happy. |
| padding = list(repeat(padding, 4)) | ||
| elif isinstance(padding, list) or isinstance(padding, tuple): | ||
| if len(padding) == 2: | ||
| padding = (*padding, *padding) |
There was a problem hiding this comment.
This is (unfortunately) not Python 3.4 syntax. Accommodating people using LTS software (including myself), we should keep this Python 3.4-compatible. Easily enough:
padding = tuple(padding * 2)
|
How would I go about enabling tests? Does this mean I move them to the Orange/tests directory, or do I have to somehow indicate that the tests should be included? |
Nevermind. Your tests are running on Travis. 😊 |
| x > 30 | ||
| >>> rule = ContinuousRule('age', True, 30) | ||
| >>> ContinuousRule('age', True, 30) | ||
| age > 30.000 |
There was a problem hiding this comment.
Sorry to have mislead you, this is not ok. __repr__() should return a Python expression from which the object can be recreated. So best use separate repr and str implementations.
There was a problem hiding this comment.
Please see if the __repr__ implementation is now all right. I've also changed the docstrings to use print() since I find this conveys the meaning of the statement
Rule ∈ [1.000, 3.000)
much clearer than this
IntervalRule(attr_name='Rule', left_rule=ContinuousRule(attr_name='Rule', greater=True, value=1, inclusive=True), right_rule=ContinuousRule(attr_name='Rule', greater=False, value=3, inclusive=False))
If this is not ok, I can fix it.
| return ContinuousRule(self.attr_name, self.greater, larger) | ||
| # When both are LT | ||
| else: | ||
| smaller = self.value if self.value < rule.value else rule.value |
There was a problem hiding this comment.
min(self.value, rule.value) ?
pavlin-policar
force-pushed
the
pythagorean-tree
branch
from
acc0cd1
to
3102f7f
Compare
|
This is almost ready to merge. I have tested it on Mac, and everything works fine. Some minor suggestions before merging:
|
|
|
||
| MAX_OPACITY = 1. | ||
| SELECTION_OPACITY = .5 | ||
| HOVER_OPACITY = .1 |
There was a problem hiding this comment.
I think this value might be too small. When one branch is hovered, the other branches are almost invisible here.
There was a problem hiding this comment.
I made it that way because for larger trees, where there is a lot of overlap (e.g. adult with sqrt sizing) the almost invisible-ness of other branches makes the branch you are hovering over very clear. If I change the hover opacity to anything larger (e.g. 0.2), it becomes less clear which branch is being highlighted.
If you feel this is unnecessary and having a higher opacity would be better I can change this.
There was a problem hiding this comment.
I see (thanks). In that case, it's ok.
…orest name camel case
Pythagorean tree widget for visualising trees. Pythagorean forest widget for visualising random forests.
Both work with the
TreeandRandomForestinterfaces introduced recently in 2230b7c.Commit history available in prototypes.
The util files I have for graphic views and components should probably be moved to a better named folder (widgetutils isn't the best, but utils was taken and my utils and the existing ones don't really fit into the same context).