fastai / fastai2

Temporary home for fastai v2 while it's being developed
https://dev.fast.ai
Apache License 2.0
645 stars 235 forks source link

Pearson and Spearman Correlation are mainly used for regression problem. #518

Closed richarddwang closed 4 years ago

richarddwang commented 4 years ago

Currently the two metric is dim_argmax=-1, but they are mainly used for regression problem (e.g. GLUE's stsb task), which pred is not logits but scores with the same shape with targets.

I move the two to Regression section, and make default dim_argmax=None for them, but users can still specify dim_argmax=-1 if they want to use them in classification problem.

review-notebook-app[bot] commented 4 years ago

Check out this pull request on  ReviewNB

Review Jupyter notebook visual diffs & provide feedback on notebooks.


Powered by ReviewNB

github-actions[bot] commented 4 years ago

👋 @richarddwang! We detected notebooks that are not stripped or that are out of sync with the library in this PR.

You need to run nbdev_install_git_hooks before submitting a pull request! For more information, see the CONTRIBUTING guide.

You can see the logs of the tests that triggered this message in run 202641237.

jph00 commented 4 years ago

Seems you removed scs_to_fastai? Could you please add tests for these when you move them?

richarddwang commented 4 years ago

Tests for Pearson and Spearman correlation coefficient has been added.

jph00 commented 4 years ago

Thanks! I think scs_to_fastai is still removed though?

richarddwang commented 4 years ago

Seems that I misunderstand something.

I added scs_to_fastai with PearsonCorrCoef and SpearmanCorrCoef metrics in a pr previously. And now I found scs_to_fastai is actually not useful and redundant so I remove it when fix the two metrics in this pr.

Is there any things I need to do now ?

jph00 commented 4 years ago

Yes please put it back. fastai has a public API. Only things with a _ suffix are private API. We don't remove things from the public API if it's not used internally - users are still using it externally.

jph00 commented 4 years ago

...well, I guess it's simple enough that maybe it's not a problem. I'll merge the PR, and if folks complain that they were using it, we can always put it back :)

richarddwang commented 4 years ago

Oh, I see. And thank you !