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

add constructor for binary classifiers #248

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from

Conversation

tiemvanderdeure
Copy link

@tiemvanderdeure tiemvanderdeure commented Apr 23, 2024

This PR adds a separate constructor NeuralNetworkBinaryClassifier for binary classification.

Unlike for the existing NeuralNetworkClassifier constructor, in the final layer always has size 1. By default, a sigmoid function is used to transform this layer. This is a pretty common thing to use neural networks for, and as far as I can tell it's not possible with the existing constructors.

I'll add tests and documentation if this PR gets some support.

PR Checklist

  • Tests are added
  • Documentation, if applicable

@tiemvanderdeure
Copy link
Author

I think the test failures are unrelated to this PR?

@ablaom
Copy link
Collaborator

ablaom commented Apr 23, 2024

@tiemvanderdeure Thanks for this PR, which looks like a valuable contribution. I'll try to investigate the fail soon and get back you.

@ablaom
Copy link
Collaborator

ablaom commented Apr 23, 2024

@tiemvanderdeure The source of your fail is sorted. Do you mind rebasing onto dev?

Copy link
Collaborator

@ablaom ablaom left a comment

Choose a reason for hiding this comment

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

Thanks @tiemvanderdeure for this contribution and taking the time to wrap your head around the code base.

I can see attention to detail in the PR, which is much appreciated. I couldn't spot any flaws in your implementation. Are you happy to add some tests?

@tiemvanderdeure
Copy link
Author

Thanks for reviewing. I'll add some tests and documentation later this week!

@ablaom
Copy link
Collaborator

ablaom commented May 1, 2024

@tiemvanderdeure I'm tweaking the tests a bit at #251. I suggest you wait till I finish that before writing your tests. Sorry for any inconvenience.

@ablaom
Copy link
Collaborator

ablaom commented May 6, 2024

Regarding tests: I don't think it is nessary to run testoptimiser or to compare predictions b/w CPU/GPU, as we do for the other classifier. The basictest should suffice.

@ablaom
Copy link
Collaborator

ablaom commented May 26, 2024

Re documentation. In view of #252 just merged, you just need to update the table at https://github.com/FluxML/MLJFlux.jl/blob/dev/docs/src/interface/Summary.md and write a model doc-string, modelled on the existing classifier.

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.

None yet

2 participants