252 feature implement orthogonal partial least squares opls algorithm#259
252 feature implement orthogonal partial least squares opls algorithm#259
Conversation
|
Hi @paucablop! I really like the level of documentation you provide which is always very good. Also like the PR template - I will definitely use that when I make PRs in the future. As I said, I have not finished the review at all, but will hopefully finish it either tomorrow or early next week! 😄 |
Super!! No rush @NusretSalli take your time! 😄 And thanks a lot 🤩 🤩 |
|
|
||
| _parameter_constraints: dict = { | ||
| "n_components": [Interval(Integral, 1, None, closed="left")], | ||
| "copy": ["boolean"], | ||
| } |
There was a problem hiding this comment.
I am wondering if there should be a check done for the number of components you can define so that n_components = min(n_samples, n_features). Otherwise users can ask for more components than the rank of X matrix, which results in Xk to be rank deficient.
| total_ss_x_k = np.sum(Xk**2) | ||
|
|
||
| # Step 11: Calculate variance ratio in prediction matrix | ||
| self.retained_variance_ratio_ = total_ss_x_k / total_ss_x |
There was a problem hiding this comment.
Even though this might be unlikely, it might be good to add a check to see if you are diving with 0. Essentially you can check if total_ss_x=0
| yk = y_centered.copy() | ||
| yk = y_centered.reshape(-1, 1) if y_centered.ndim == 1 else y_centered | ||
|
|
There was a problem hiding this comment.
The first yk call is getting overwritten by the next yk variable declaration, making the first call redundant
| # Step 7: Calculate sum of squares in defleated Xk | ||
| total_ss_x_k = np.sum(X_deflated**2) | ||
|
|
||
| # Step 8: Calculate variance ratio in prediction matrix | ||
| self.retained_variance_ratio_ = total_ss_x_k / total_ss_x |
There was a problem hiding this comment.
Same feedback as in _orthogonal_pls.py
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Nusret Emirhan Salli <110097112+NusretSalli@users.noreply.github.com>
Co-authored-by: Nusret Emirhan Salli <110097112+NusretSalli@users.noreply.github.com>
Summary
This pull request implements:
Why is this change needed?
OPLS and DO are part of a family of filters commonly used in spectroscopic analysis
Closes
Related issues
Type of change
What changed?
What did NOT change?
API and compatibility impact
Notes
scikit-learnAPI compatibility: all confirmedValidation
task format-checktask linttask spellingtask type-checktask testtask coveragetask test:matrixtask docs:checktask buildValidation notes
NA
Tests
Test coverage details
projection/*Documentation
Documentation notes
OSC, DO, EPO and OPLS docstrings were updated. OPLS and DO were added to the documentation page and are displayed.
Dependency / build / CI impact
Notes
ty=>0.0.30Reviewer focus
Please focus on:
Checklist