Add Neighbor Joining widget#286
Conversation
markotoplak
force-pushed
the
add-neighbor-joining-widget
branch
from
35a95d6 to
6a956e4
Compare
|
Thanks! I see the tests run well, this is good. Then, your PR is mostly based on some existing code form biolab/orange3. Thus, I split it into two separate commits: one that just copied the existing code from biolab/orange3's master branch, and then added your files as they were. This makes your changes easier to trace in could be useful in the future if we decide to integrate this into the main Orange. |
| self.Error.no_numeric_features() | ||
| return | ||
| self._set_valid_matrix(matrix) | ||
| except Exception as e: |
There was a problem hiding this comment.
Catching all exceptions and displaying them as errors is usually a bad practice. If something is not supposed to crash and you have relatively good control, then it is ever beneficial if an actual exception happens: users get notified and could report it.
| return | ||
| self._set_valid_matrix(matrix) | ||
| except Exception as e: | ||
| self.Error.distance_computation_error(error=str(e)) |
There was a problem hiding this comment.
Catching all exceptions and displaying them as errors is usually a bad practice. If something is not supposed to crash and you have relatively good control, then it is ever beneficial if an actual exception happens: users get notified and could report it.
| self._set_displayed_root(prune(tree, level=self.max_depth)) | ||
| else: | ||
| self._set_displayed_root(tree) | ||
| except Exception as ex: |
| elif self.annotation == "Name": | ||
| attr = self.matrix.row_items.domain.attributes | ||
| labels = [str(attr[i]) for i in indices] | ||
| row_items = self.matrix.row_items |
There was a problem hiding this comment.
Why did the logic change here, for annotation? Why were changes here needed at all - in the and, the annotatio are shown similarly.
| labels = new_labels | ||
|
|
||
| if not self.pruning and self.color_by is not None: | ||
| # Compute per-leaf colors (for the small rectangle) and subset bold mask |
There was a problem hiding this comment.
For me it is not obvious why these parts had to be changed.
| if root: | ||
| clusters = top_clusters(root, n) | ||
| self.dendrogram.set_selected_clusters(clusters) | ||
| if not root: |
There was a problem hiding this comment.
This function is an example of a function that has good arguments why it needs to be different to NJ
| self.Error.distance_computation_error(error=str(e)) | ||
|
|
||
| @Inputs.data | ||
| def set_data(self, data): |
There was a problem hiding this comment.
Adding Data should be handled carefully. I'd prefer not to have it, because it contains too many automatic choices that are not transparent.
At minimum it should be transparent. So, if it made a choice, that choice would have to be reported to the user in some kind of an info box. Then, it assumes a specific axis, which is a big assumption, and it does not standardize the data. In such cases the proper thing would be to have a combo where the user would select the axis (rows/colums) and a normalization/standardization check box, because these are important parameters. But if you added all these, then there is no gain compared to using explicit Distance.
I am commenting this from the point of view of Orange UI design principles.
|
A deficiency of this widget is that selections are not saved. I saw the code handling selection saving was removed (from the HC code) intentionally, and I understand the simplification for a prototype. Selection saving is otherwise very important in Orange, because it enables users to share workflows - and if selection was saved, also widgets downstream of Neighbor Joining would show the same thing. Now they would not. But this is fine for prototype. Looking at code I saw quite a few changes from the original Hierarchical clustering that should not be necessary - in NJ, annotation columns and colors code was change substantially. Also the code for drawing does seem a bit complex given the functionality. But, OK, given its prototype status. For me, the only blocker to merging is the added Data input, which I explained in a code comment. I can remove it myself - or, alternatively, you can do that. Please tell me how you decide. |
2 participants
Issue
Adds a prototype Neighbor Joining widget.
Description of changes
Added files:
neighbor_joining.pyImplements the core algorithm,
TreeNodestructure, leaf ordering helpers and Newick export.neighbor_joining_adapter.pyConverts the
TreeNodestructure into Orange'shierarchical.Treestructure.dendrogram_nj.pyCustom dendrogram component, supports non-ultrametric trees by preserving leaf heights and branch lengths.
owneighborjoining.pyAdds the Orange widget for visualizing, pruning, selecting, and outputting clusters.
test_neighbor_joining.pyTests for core algorithm and tree adapter.
test_owneighborjoining.pyWidget tests for input handling, error states, tree construction, and outputs.
Includes