factor analysis: new widget

[suriikata]

Issue

New widget for Factor Analysis

Includes

• Code changes
• Tests
• Documentation

[codecov-commenter]

Codecov Report

Merging #251 (542b61a) into master (a103963) will decrease coverage by 1.80%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master     #251      +/-   ##
==========================================
- Coverage   36.00%   34.19%   -1.81%     
==========================================
  Files           6        7       +1     
  Lines        1175     1237      +62     
==========================================
  Hits          423      423              
- Misses        752      814      +62     

Impacted Files	Coverage Δ
...angecontrib/prototypes/widgets/owfactoranalysis.py	0.00% <0.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a103963...542b61a. Read the comment docs.

[janezd]

I turned this exercise into an advanced programming course with lots of stylistic comments. :)

There's one more design thing that I haven't included, because it is related to multiple places in the code: what the Apply button and the deferred commits are supposed to do. Please remind me to explain it on Friday.

[janezd]

This is OK, but I don't think any new widget uses it. It is cleaner to write a method like this:

def _n_components_changed(self):
    self.factor_analysis()
    self.commit.deferred()

and then use callback=_n_components_changed.

Allowing specifying a list as callback was one of many bad ideas I had 15 years ago.

[suriikata]

why is it a bad idea?🙃

[janezd]

I can't put it into words. :) The code is usually cleaner if the action that needs to occur in response to, say, a changed value of spin, is described in a function.

Perhaps one reason is also that you can give the function a descriptive name, like _on_components_spin_changed. Aleš, whose code is a few levels above any of ours, does it this way and it looks really neat. In his code, there is a section with handlers for various events, and you can just go there and see everything. If you just put it into lists, it soon becomes messy.

I recently tried to imitate this here: https://github.com/biolab/orange3-timeseries/blob/master/orangecontrib/timeseries/widgets/owmovingtransform.py#L337.

[janezd]

This has no effect, nobody sees it. In this case, you can assume this won't happen, so you can just change the last elif to else (well, you don't have to because you don't need if-s anyway).

There is however a pattern you can sometimes use for cases where something shouldn't happen and probably doesn't, but you'd like to check it and are OK with widget crashing if it does happen (because the widget, for instance, can't continue). In your case, if you'd like to do this, you'd put

assert self.setting_for_rotation <= 2 before if-s. If the condition is not true, the widget will fail with assertion error.

That said, don't do this here, because setting_for_rotation will always be correct.

For a good example, OWPredictions attribute self.data. If self.data is not None, it constructs ... some model (whatever). Now, a method _commit_predictions immediately exits if there is no data:

        if not self.data:
            return

Later in this function, you can see

datamodel = self.dataview.model()

This datamodel is the model, which must and does and will exist if self.data exists. So datamodel cannot be None. But this may not be immediately obvious to people who read this code, so the code continues with

assert datamodel is not None  # because we have data

First, this tells the reader: don't worry, we're not checking that datamodel is not None, because it can't be, and the comment tells why.

Second, if there is a bug in the code, the assertion will fail with AssertionError. This doesn't help the user at all -- but the widget would fail later anyway, and failing here makes it easier to find and understand the problem if it occurs.

Gosh, you're getting an advanced course in programming in these comments. :)

Anyway, using assertions is good and we should use them more often.

[suriikata]

thanks for the in depth explanation, very useful!

[suriikata]

thank you for writing such extensive suggestions, it was a very valuable lesson!🐝

[janezd]

I fear this has been stalled, and if it is implemented, we'll start from scratch...