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

fix: bug in OPLS predict method + testcase #25

Merged
merged 4 commits into from
Oct 20, 2021
Merged

fix: bug in OPLS predict method + testcase #25

merged 4 commits into from
Oct 20, 2021

Conversation

josoriom
Copy link
Member

No description provided.

@codecov
Copy link

codecov bot commented Oct 14, 2021

Codecov Report

Merging #25 (d46ef00) into master (45a67a4) will increase coverage by 1.26%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #25      +/-   ##
==========================================
+ Coverage   94.51%   95.78%   +1.26%     
==========================================
  Files           6        6              
  Lines         529      546      +17     
  Branches       62       80      +18     
==========================================
+ Hits          500      523      +23     
+ Misses         28       23       -5     
+ Partials        1        0       -1     
Impacted Files Coverage Δ
src/OPLS.js 95.88% <100.00%> (+2.81%) ⬆️
src/oplsNipals.js 100.00% <0.00%> (ø)
src/PLS.js 97.41% <0.00%> (+0.02%) ⬆️
src/KOPLS.js 95.61% <0.00%> (+0.03%) ⬆️
src/util/utils.js 73.68% <0.00%> (+1.46%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 45a67a4...d46ef00. Read the comment docs.

src/OPLS.js Outdated Show resolved Hide resolved
features.scale('column', { scale: this.stdevs });
if (labels.rows > 0 && this.mode === 'regression') {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why aren't you scaling the labels on regression's mode?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed I'm doing it (between lines 341 and 343). But this if in those lines are because the labels are optional (if you know which class the sample belongs) in order to calculate statistical parameters Q2y and AUC and that's the bug that I want to fix, some cases I want to test a sample without knowing which class belongs, and that's not currently working.

image

src/OPLS.js Outdated Show resolved Hide resolved
src/OPLS.js Outdated Show resolved Hide resolved
src/OPLS.js Outdated
@@ -313,35 +313,40 @@ export class OPLS {
* @param {Number} [options.nc] - the number of components to be used
* @return {Object} - predictions
*/
predict(newData, options = {}) {
const { trueLabels = [] } = options;
predict(data, options = {}) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please could you is jsdoc:

  • specify that [options={}]
  • use the same name in the method as in the jsdoc (so either data or features)

src/OPLS.js Outdated
}

const features = new Matrix(newData);
const features = new Matrix(data);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for me in the call of the method you may replace data by features and just specify features = new Matrix(features) to ensure it is a Matrix

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@lpatiny lpatiny merged commit 226d4c4 into mljs:master Oct 20, 2021
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