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

[ENH] PLS: Move from Orange-spectroscopy #6734

Merged
merged 14 commits into from
Mar 22, 2024
Merged

Conversation

VesnaT
Copy link
Contributor

@VesnaT VesnaT commented Feb 16, 2024

Issue

Move PLS widget from Orange-spectroscopy.

Includes
  • Code changes
  • Tests
  • Documentation

@VesnaT VesnaT marked this pull request as draft February 16, 2024 09:55
Copy link

codecov bot commented Feb 16, 2024

Codecov Report

Merging #6734 (c21b95a) into master (88f1048) will increase coverage by 0.02%.
Report is 21 commits behind head on master.
The diff coverage is 96.19%.

❗ Current head c21b95a differs from pull request most recent head 0144062. Consider uploading reports for the commit 0144062 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6734      +/-   ##
==========================================
+ Coverage   88.14%   88.16%   +0.02%     
==========================================
  Files         322      325       +3     
  Lines       70586    70773     +187     
==========================================
+ Hits        62215    62398     +183     
- Misses       8371     8375       +4     

@borondics
Copy link
Member

Thanks guys!!!

@VesnaT VesnaT marked this pull request as ready for review February 16, 2024 12:12
@markotoplak
Copy link
Member

I particularly don't like how ellipsis is implemented because it is hardcoded into PLS and forces the first two components.

I saw that people wish for something similar also for PCA, and, also sometimes they are interested in other components, for example, 2nd vs 3rd.

If I understand the idea behind it, a simple multivariate (ok, 2-variate) distribution is fitted on the data set, and then the ellipse of a certain percentage can be drawn. The code by @VesnaT only needs two columns of data, and I doubt it requires any assumptions only covered by PLS. So it should be general.

I see two ways of implementing it:

  1. A separate widget that allows users to select two features and then adds the column.
  2. Add these (general) ellipses to the Scatter Plot.

I would go for the second option. I was also asked quite a few times why we don't have this. Also, these ellipses fit into scatter plot visualization at least as much as regression lines (and even better than those that do not treat variables as independent).

@markotoplak markotoplak added the needs discussion Core developers need to discuss the issue label Feb 29, 2024
@markotoplak
Copy link
Member

We discussed this and decided on adding the ellipse to the scatter plot widget.

@markotoplak markotoplak removed the needs discussion Core developers need to discuss the issue label Mar 1, 2024
@VesnaT VesnaT mentioned this pull request Mar 4, 2024
3 tasks
@markotoplak markotoplak changed the title PLS: Move from Orange-spectroscopy [ENH] PLS: Move from Orange-spectroscopy Mar 4, 2024
@markotoplak markotoplak merged commit 941cd6c into biolab:master Mar 22, 2024
18 of 25 checks passed
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.

3 participants