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

Scatterplot branch #30

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Conversation

jreus
Copy link

@jreus jreus commented Jan 14, 2020

No description provided.



/*
T for Transform
Copy link
Contributor

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; };
Copy link
Contributor

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

Copy link
Contributor

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|
Copy link
Contributor

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 {
Copy link
Contributor

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 {
Copy link
Contributor

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?

@joslloand
Copy link
Contributor

One thing of concern I'm spotting is:

https://github.com/supercollider-quarks/MathLib/pull/30/files#diff-41d12296e562fa6d5b6b604bbc2df9023cdf7384a4c93085472d0458ef822859L210-R212

This change would deprecate the current behavior of Matrix-collect and replace with Matrix-collectRowsCols. Any call to Matrix-collect would then invoke Collection-collect, which would not return the same result.

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 Matrix-collect.

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:

  • rename the deprecate method as theMethod_old
  • add the new method
  • throw a deprecation warning for the deprecated method, re-directing in the throw to the new method

Ah... actually, here's a short note: use Object-deprecated(method, alternateMethod)

@dyfer
Copy link
Contributor

dyfer commented Oct 17, 2020

I agree that the old method should be deprecated and redirect to the new method.
BTW any policies we have in SC w/r/t deprecations do not necessarily apply to quarks (as there's no strong connection between SC and quarks' versioning). But it does make sense to not simply remove methods and thus break users' code.

Example of deprecations can be found in SCClassLibrary/deprecated, e.g.:

asInt {
	this.deprecated(thisMethod, this.class.findMethod(\asInteger));
	^this.asInteger;
}

@redFrik redFrik mentioned this pull request Jan 11, 2022
@dyfer
Copy link
Contributor

dyfer commented Apr 18, 2022

Hello @jreus do you intend on addressing review comments in this PR?
Additionally, master branch now includes fixes to ScatterView so these should be removed from this PR.

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.

4 participants