Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
factor analysis: new widget #251
base: master
Are you sure you want to change the base?
factor analysis: new widget #251
Changes from 2 commits
542b61a
2b7e29b
939a004
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is OK, but I don't think any new widget uses it. It is cleaner to write a method like this:
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is it a bad idea?🙃
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You won't use
SliderGraph
, so this is moot, but: if you need a no-op function, uselambda x: None
(this would be a function that accepts one argument and does nothing).By the way, the whole point of
SliderGraph
is to call this function when the slider moves. Which proves that you don't needSliderGraph
here. :)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i replaced self.prazna_funkcija, but i will leave this unresolved until i figure out how to replace the SliderGraph🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder ... this creates a different scaling for x and y axes. Maybe you'd have to use the same (that is the larger) factor for both?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my concern is that, if one factor is much larger than the other, having the same scale would make the smaller one difficult to see.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this would be correct. If one is negligible in comparison with another, it should also look negligible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A nicer way to organize this function would be to first set the FA method and then call it (because the call is same for all).
Also, you have defined the constants for rotations, so you can use
NoRotation
instead of0
etc.However, all that differs is the value of the argument rotation. You can get rid of
if
-s, for instance like this:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
toelse
(well, you don't have to because you don't needif
-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
beforeif
-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
attributeself.data
. Ifself.data
is notNone
, it constructs ... some model (whatever). Now, a method_commit_predictions
immediately exits if there is no data:Later in this function, you can see
This
datamodel
is the model, which must and does and will exist ifself.data
exists. Sodatamodel
cannot beNone
. But this may not be immediately obvious to people who read this code, so the code continues withFirst, 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the in depth explanation, very useful!