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.

Sign up for GitHub

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

Jump to bottom

Added activation functions to MLP models. #45

Merged
merged 9 commits into from
May 31, 2024
Merged

Added activation functions to MLP models. #45

merged 9 commits into from
May 31, 2024

Conversation

jdenholm
Copy link
Collaborator

@jdenholm jdenholm commented Apr 6, 2024 •
edited

  • Update the penguin classification demo by adding activation functions to the model.
  • Update the penguin regression demo.

Closes #43.

@jdenholm jdenholm added the documentation Improvements or additions to documentation label Apr 6, 2024
@jdenholm jdenholm marked this pull request as draft April 6, 2024 12:56
jatkinson1000
jatkinson1000 reviewed Apr 7, 2024
View reviewed changes
Copy link
Member

@jatkinson1000 jatkinson1000 left a comment

Choose a reason for hiding this comment

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

Generally looks good.
Couple of questions for my own understanding and couple of comments.
Once you look I'd be happy to merge.
Are you coming to the pub Thursday?

pyproject.toml Outdated Show resolved Hide resolved
worked-solutions/01_penguin_classification_solutions.ipynb Show resolved Hide resolved
worked-solutions/01_penguin_classification_solutions.ipynb Outdated Show resolved Hide resolved
worked-solutions/01_penguin_classification_solutions.ipynb Show resolved Hide resolved
worked-solutions/01_penguin_classification_solutions.ipynb Outdated Show resolved Hide resolved
worked-solutions/02_penguin_regression_solutions.ipynb Show resolved Hide resolved
@jatkinson1000
Copy link
Member

Hey @jdenholm is this ready to be merged do you think? It looks like the JOSE review is nearly passed so it would be good to merge this before that.

Aside from the point about dependencies I think the rest of my comments are purely questions for my own interest so no major objections.

@jdenholm
Copy link
Collaborator Author

I'm happy for you to change it as you please after my replies. Sorry for taking an age.

jatkinson1000
jatkinson1000 approved these changes May 31, 2024
View reviewed changes
Copy link
Member

@jatkinson1000 jatkinson1000 left a comment

Choose a reason for hiding this comment

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

This is great, thanks for answering my questions @jdenholm.
I have addressed the minor points I made with a couple of follow-up commits and now all LGTM so will merge.

@jatkinson1000 jatkinson1000 marked this pull request as ready for review May 31, 2024 07:58
@jatkinson1000 jatkinson1000 merged commit 736e054 into main May 31, 2024
1 check passed
@jatkinson1000 jatkinson1000 deleted the fc-nets branch May 31, 2024 07:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jatkinson1000 jatkinson1000 jatkinson1000 approved these changes

Assignees

@jdenholm jdenholm

@jatkinson1000 jatkinson1000

Labels
documentation Improvements or additions to documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Changes to network achitectures
2 participants
@jdenholm @jatkinson1000