-
Notifications
You must be signed in to change notification settings - Fork 10
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
Scatterplot branch #30
base: master
Are you sure you want to change the base?
Conversation
|
||
|
||
/* | ||
T for Transform |
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.
often, the need to explain is an indication that a name could be improved. If this class is not used dense repetiton, like classes like Array
or Event
, you could just as well write it out.
TransformNormalizer
This would be better because T
may mean a lot of different things (think of task inTdef
), and Normalizer
is a UGen.
Also possible is to make the verb the name of a class:
NormalizeTransform
Both I find fine.
} | ||
|
||
init {|dataset| | ||
if(dataset.isKindOf(SequenceableCollection).not ) { "Dataset must be a Matrix".error;this.halt; }; |
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.
Better write:
Error("Dataset must be a Matrix".).throw
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.
btw. the test .isKindOf(SequenceableCollection)
doesn't exclude things like a Set
. So if you pass a Set
it would go through.
|
||
// denormalize a point-slope form 2-dimensional line | ||
// of the form [slope, intercept] | ||
denormalizeLine {|line| |
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.
can you check the formatting of whitespace? Please use tabs not spaces, and auto-formatting.
|
||
T is for Transform | ||
*/ | ||
TStandardizer { |
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.
TransformStandardizer
or StandardizeTransform
?
/* | ||
T for Transform | ||
*/ | ||
TPCA { |
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.
hm? This class is just an object?
update from main repo
One thing of concern I'm spotting is: This change would deprecate the current behavior of I'm not opposed to the use of a more descriptive method name, but, such a change without a deprecation warning could easily break existing code that that uses Having a very quick look on the SC wiki I'm not finding instructions for deprecations. I seem to recall the thing to do is:
Ah... actually, here's a short note: use |
I agree that the old method should be deprecated and redirect to the new method. Example of deprecations can be found in asInt {
this.deprecated(thisMethod, this.class.findMethod(\asInteger));
^this.asInteger;
} |
Hello @jreus do you intend on addressing review comments in this PR? |
No description provided.