-
Notifications
You must be signed in to change notification settings - Fork 34
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
added new metrics for regression tasks #364
Conversation
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.
Looks great!
Minors inline.
), f"Spearman corr expected to get pred and target with same length but got pred={len(pred)} - target={len(target)}" | ||
|
||
statistic, p_value = spearmanr( | ||
pred, target, nan_policy="propagate" |
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 we expose nan_policy to be an argument in metric class?
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.
it made sense to me to make this visibility to understand where nans might come from but i don't mind removing
) | ||
|
||
|
||
class MetricSpearmanCorrelation(MetricDefault): |
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 suggest that all of those metrics (including MetricPearsonCorrelation) be put in a new folder for regression. Makes sense?
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.
yes makes a lot of sense, will refractor
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.
LGTM
FYI @IdoAmosIBM @mosheraboh here's the PR - #365 |
I don?t see an option to approve the PR so will have to be @moshiko ***@***.***>
From: YoelShoshan ***@***.***>
Date: Wednesday, 11 September 2024 at 15:09
To: BiomedSciAI/fuse-med-ml ***@***.***>
Cc: Ido Amos ***@***.***>, Mention ***@***.***>
Subject: [EXTERNAL] Re: [BiomedSciAI/fuse-med-ml] added new metrics for regression tasks (PR #364)
FYI @IdoAmosIBM @mosheraboh Note that as a result of this PR some tests in dependent libs failed, as it relied on imports from a certain sub dir/module. For backward compatibility I'm making them accessible from the same location (do tell me
FYI
@IdoAmosIBM<https://github.com/IdoAmosIBM> @mosheraboh<https://github.com/mosheraboh>
Note that as a result of this PR some tests in dependent libs failed, as it relied on imports from a certain sub dir/module.
For backward compatibility I'm making them accessible from the same location (do tell me if you prefer something else)
here's the PR - #365<#365>
?
Reply to this email directly, view it on GitHub<#364 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/BBVNEQTV6BRATOJS6OFJ72TZWAXFNAVCNFSM6AAAAABN6LFRNCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNBTGQ4TCMJYGA>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
No description provided.