-
Notifications
You must be signed in to change notification settings - Fork 17
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
Center biplot #27
Center biplot #27
Conversation
|
||
# Sample Loadings | ||
sample_loading = pd.DataFrame(opt.sample_weights, index=table.index) | ||
sample_loading = sample_loading.rename(columns=rename_cols) | ||
sample_loading -= sample_loading.mean(axis=0) |
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 doesn't have to be resolved now, but for the lucky future developer - it looks like there is some redundancy here compared to _method.py. May want to redefine some functions to avoid copy-pasta fixes 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.
definitely. need to merge between them.
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.
Another note for a future developer, there will eventually need to have unittests to test the contents of feature loadings and sample loadings -- this will become more important for regression testing to catch future bugs.
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.
These notes/suggestion will be lost - what about creating specific issues in the repo and adding a full description of what needs to be done + pointing here?
@cameronmartino - don't forget to include a snapshot of the emperor plot before we merge, so that we can take a note that this issue has been resolved. |
@mortonjt good point. |
Hmm - can't seem to see the after image ... |
Ok - looks good. @cameronmartino don't forget to update the changelog and then I can merge this in. |
resolves issue #26