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

[ENH] Linear Projection (LDA, PCA) #2445

Merged
merged 6 commits into from
Dec 11, 2017
Merged

[ENH] Linear Projection (LDA, PCA) #2445

merged 6 commits into from
Dec 11, 2017

Conversation

jerneju
Copy link
Contributor

@jerneju jerneju commented Jun 30, 2017

Issue
Description of changes
  1. Merged Linear Projection from Orange3, Linear Projection from Prototypes
  2. LDA projection
  3. PCA projection (NEW)
  4. Input projections
  5. Rank for LDA and Circular projections.
  6. Now use Scatter Plot Graph instead of Plotutils
  7. Added some new tests
  8. Updated documentation
Includes
  • Code changes
  • Tests
  • Documentation

@codecov-io
Copy link

codecov-io commented Jul 7, 2017

Codecov Report

Merging #2445 into master will decrease coverage by 5.12%.
The diff coverage is 88.38%.

@@            Coverage Diff             @@
##           master    #2445      +/-   ##
==========================================
- Coverage   81.68%   76.55%   -5.13%     
==========================================
  Files         323      337      +14     
  Lines       55450    59664    +4214     
==========================================
+ Hits        45293    45678     +385     
- Misses      10157    13986    +3829

@@ -0,0 +1,348 @@

import numpy
Copy link
Contributor

Choose a reason for hiding this comment

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

Import numpy as np.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, thanks. I always do replace numpy with np. Mostly before changing anything else. However this file is just a copy from Prototypes.

@janezd
Copy link
Contributor

janezd commented Aug 22, 2017

What is the status of this PR? Since we later decided for separate widgets, is this PR still relevant?

@jerneju
Copy link
Contributor Author

jerneju commented Aug 23, 2017

Yes, this PR is still relevant. I have edited the first comment, see above.

@jerneju jerneju changed the title [WIP] Linear Projection [WIP] Linear Projection (LDA, PCA) Sep 15, 2017
@jerneju
Copy link
Contributor Author

jerneju commented Sep 29, 2017

@janezd
When GUI is approved, I will updated the documentation.

@jerneju jerneju changed the title [WIP] Linear Projection (LDA, PCA) Linear Projection (LDA, PCA) Oct 4, 2017
@jerneju
Copy link
Contributor Author

jerneju commented Oct 9, 2017

@janezd

janezd
janezd previously requested changes Oct 13, 2017
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 am reviewing this for some time. I am going up and down through the code, trying to get some sense of what is actually needed and what are scattered remains of previous reincarnations and designs of the widget. The comments are unfortunately ordered by lines and not chronologically, which would in this case make more sense (or at least illustrate my struggle here).

My reviewing stops here. I'm sorry, but this code is such a mess that I can' t provide a useful review. I'm marking it as WIP. Please remove this flag when the code is ready for a proper review.

SIZE_POLICY_FIXED = (QSizePolicy.Minimum, QSizePolicy.Maximum)


class Hidden(QDialog):
Copy link
Contributor

Choose a reason for hiding this comment

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

This name is uninformative. This is essentially and AddVariablesDialog or sth like that.

)
self.btn_select = QPushButton(
"Select", autoDefault=False, sizePolicy=QSizePolicy(*SIZE_POLICY_FIXED)
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Rename this button to Add.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is not a modal dialog. It is modal in the sense that is blocks other dialogs, but it does not function as one from the usage perspective. As it is now, the user must select attributes, click "Select" (after actually selecting them!) to add them. Then (s)he can add more. After (s)he is done, she has to click Cancel to essentially confirm what (s) has done.

Implement it like this: the widget calls this dialog and receives a list of features to add. That is, the dialog has two buttons, Add and Cancel. The user selects the features and clicks Add to confirm or Cancel to cancel. After this, the dialog closes and the features are added. See if you can use some standard Qt stuff so that placement of buttons is correct wrt the platform; I'm not sure this is possible, though.

Make Add a default button.

self.btn_select = QPushButton(
"Select", autoDefault=False, sizePolicy=QSizePolicy(*SIZE_POLICY_FIXED)
)
self.btn_select.clicked.connect(master.display_select)
Copy link
Contributor

Choose a reason for hiding this comment

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

... and this magic will no longer be needed.

self.layout().addWidget(master.other_view)
self.layout().addWidget(btns_area)

def __call__(self, master):
Copy link
Contributor

Choose a reason for hiding this comment

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

Do it more in a style of Qt's standard dialogs. If you do it properly, it may even be a function, I guess.

self.master.master.setEnabled(True)
super().closeEvent(QCloseEvent)

class VariableSelection(QObject):
Copy link
Contributor

Choose a reason for hiding this comment

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

How much of this code is still needed? Accepting drops looks like functionality that we only needed when the widget still had two list views.

This looks like a huge class for what is essentially a decorated listview with ordering.


view.setModel(model)
self.filter_edit, self.other_view = variables_filter(
model=model)
Copy link
Contributor

Choose a reason for hiding this comment

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

self.other_view is set here ... but also a few lines earlier. What is self.other_view?!

view.setModel(model)
self.filter_edit, self.other_view = variables_filter(
model=model)
self.hidden = Hidden(self)
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe that a huge part of the mess related to models comes from having Hidden as a persistent dialog. Make it a normal modal dialog that is created when needed, and simply initialized with all attributes appearing in the domain that are currently hidden.

"Move up", view,
shortcut=QKeySequence(Qt.Key_Delete),
triggered=self.__activate_selection
)
Copy link
Contributor

Choose a reason for hiding this comment

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

The delete key invoked action "Move up", which calls method __activate_selection, which deletes a variable? Should something be renamed here, or am I getting something wrong?

...

Ah, OK, after checking the action attached to the minus button, I see. Move up refers to moving to a list view that used to be above some dialog. And activation refers to showing the selected variables in the plot.

for i in sorted((ind.row() for ind in indices), reverse=True):
del model[i]

self.varmodel_other.extend(variables)
Copy link
Contributor

Choose a reason for hiding this comment

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

And this is needed just because the dialog with hidden attributes is persistent. The reason why it is persistent is that the list view used to be persistent in the previous GUI.

self.hidden(self)

@staticmethod
def encode_var_state(lists):
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these encodes and decodes really needed? Cannot settings take care of lists of variables? Is this again some legacy from the previous form of the widget?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, they are needed.

@janezd janezd changed the title Linear Projection (LDA, PCA) [WIP] Linear Projection (LDA, PCA) Oct 13, 2017
@janezd
Copy link
Contributor

janezd commented Oct 13, 2017

One more thing: the layout. Rank button is currently above the list view, while the +- buttons have empty space on the right. They are also below the box instead of in it.

screen shot 2017-10-13 at 21 32 31

Since ranking is closely related to +-, it should be there. Like this:

screen shot 2017-10-13 at 18 49 20

I'm attaching a patch that will do some of the job.

patch.zip

@jerneju jerneju changed the title [WIP] Linear Projection (LDA, PCA) [WIP] [ENH] Linear Projection (LDA, PCA) Nov 10, 2017
@jerneju jerneju changed the title [WIP] [ENH] Linear Projection (LDA, PCA) [ENH] Linear Projection (LDA, PCA) Nov 16, 2017
PCA=2,
Projection=3),
type=int,
qualname="Placement")
Copy link
Contributor

Choose a reason for hiding this comment

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

pickle.PicklingError: Can't pickle <enum 'Placement'>: attribute lookup Placement on Orange.widgets.visualize.owlinearprojection failed

Perhaps should be qualname+'OWLinearProjection.Placement'.

@jerneju jerneju modified the milestones: 3.8, 3.9 Nov 30, 2017
@kernc kernc requested a review from ajdapretnar November 30, 2017 16:40
Copy link
Contributor

@ajdapretnar ajdapretnar left a comment

Choose a reason for hiding this comment

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

I vote for updating the Example part, too. Now the workflow has three different outputs that are not explained anywhere. Add a line commenting on what these outputs mean/do/work in the analysis.
It would also be nice to add an example on how to work with a custom projection.

@@ -18,17 +18,37 @@ Signals

A subset of data instances

- **Projection**

Custom projection vectors
Copy link
Contributor

Choose a reason for hiding this comment

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

De-indent.

of class-labeled data. It supports various types of projections such as circular,
`linear discriminant analysis <https://en.wikipedia.org/wiki/Linear_discriminant_analysis>`_,
`principal component analysis <https://en.wikipedia.org/wiki/Principal_component_analysis>`_,
and custom projection
Copy link
Contributor

Choose a reason for hiding this comment

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

a custom projection. Add a full stop.

2. Optimize your projection by using **Suggest Features**. This feature
scores attributes by average classification accuracy and returns the
top scoring attributes with a simultaneous visualization update.
3. Choose the type of projection.
Copy link
Contributor

Choose a reason for hiding this comment

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

Indenting is all off. Please fix. Never forget to check how the docs look like when built with make html. Sphinx will report errors in docs. You also need to pip install CommonMark==0.5.5. Weird stuff. 🤷‍♀️


- *Label only selected points* allows you to select individual data instances and label them.

7. *Select, zoom, pan and zoom to fit* are the options for exploring the graph.
Copy link
Contributor

Choose a reason for hiding this comment

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

and should not be in italics

- *Label only selected points* allows you to select individual data instances and label them.

7. *Select, zoom, pan and zoom to fit* are the options for exploring the graph.
The manual selection of data instances works as an angular/square
Copy link
Contributor

Choose a reason for hiding this comment

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

Manual selection (omit the)

Copy link
Contributor

@lanzagar lanzagar left a comment

Choose a reason for hiding this comment

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

  • Use iris, enable class density in Linear projection and switch between different projections => only part of the picture is colored.
  • Show axes has a radius slider. When I increase it, the axes start disappearing which is counterintuitive for a "Show axes" control. Maybe rename to "Hide axes"? (@janezd any better suggestions?)

Use ScatterPlotGraph
add VizRank (Suggest Features)
LDA, PCA, and Input Projection
New Warnings
Migrate settings and context
Always commit as metas
new Scatter Plot Graph compatibility
fix PCA
selection fix
fix vizrank
lint
@lanzagar lanzagar merged commit 2c54035 into biolab:master Dec 11, 2017
@jerneju jerneju deleted the new-linearprojection branch December 11, 2017 14:48
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.

6 participants