factor analysis: new widget #251
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
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.
| callback=[self.factor_analysis, self.commit.deferred], #deferred = zapoznelo | ||
| self.attr_box, self, "n_components", label="Number of components:", | ||
| minv=1, maxv=100, step=1, controlWidth=30, | ||
| callback=[self.factor_analysis, self.commit.deferred], # deferred = zapoznelo |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| elif self.setting_for_rotation == 2: | ||
| result = FactorAnalysis(self.n_components, rotation="quartimax").fit(self.dataset.X) | ||
| else: | ||
| print("Error:") |
There was a problem hiding this comment.
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 dataFirst, 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.
There was a problem hiding this comment.
thanks for the in depth explanation, very useful!
|
thank you for writing such extensive suggestions, it was a very valuable lesson!🐝 |
Issue
New widget for Factor Analysis
Includes