-
Notifications
You must be signed in to change notification settings - Fork 9
better updates to ScatterView classes #5
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
Conversation
|
why did you open a new PR? You can just push to the branch of the old PR... |
|
@LFSaw can you briefly check if this can be merged (finally!)? |
|
if you want me to review this, please create a custom branch with a name not being master since this makes my git ecosystem choke... |
LFSaw
left a comment
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.
please remove
.gitignore
Outdated
| @@ -0,0 +1 @@ | |||
| .DS_Store | |||
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.
please remove .DS_Store
| @@ -0,0 +1,122 @@ | |||
| /***************************************** | |||
| Data Transforms | |||
| (C) 2018 Jonathan Reus | |||
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.
should this be GPL?
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.
Should also add Jonathan Reus as a contributor in the README.
| 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.
are these doing anything?
| (vals.size).min(this.cols).do({ arg col; | ||
| this.put(row, col, vals.at(col)) | ||
| });"Warning: wrong number of vals".postln; | ||
| });"Warning: wrong number of vals: % received and % expected".format(vals.size, this.cols).postln; |
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'd prefer these in new lines if possible
| (vals.size).min(this.rows).do({ arg row; | ||
| this.put(row, col, vals.at(row)) | ||
| });"Warning: wrong number of vals".postln; | ||
| });"Warning: wrong number of vals: % received and % expected".format(vals.size, this.cols).postln; |
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'd prefer these in new lines if possible
|
@jreus could you please rename the branch? "master" is indeed not a good name! :) |
patrickdupuis
left a comment
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.
the copyright needs to be removed from this PR before it can be merged.
the copyright isn't a problem apparently: https://www.gnu.org/licenses/gpl-faq.html#AssignCopyright
|
@telephon Sorry for the long delay! I made a new branch called 'scatterplot' and submitted a pull request. Also resolved the .DS_Store conflict in .gitignore |
|
closed in favour of #30 |
updated ScatterView classes for Qt (on SC 3.9.3)
also added a grid to ScatterView and some small additional styling options