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

[SYSTEMDS-3694] Sequence primitive for neural network layers #2025

Closed
wants to merge 7 commits into from

Conversation

Nakroma
Copy link
Contributor

@Nakroma Nakroma commented May 28, 2024

Draft for the student project SYSTEMDS-3694.

  • Adds a Layer interface to the Python API which can be extended by nn layers
  • Adds the sequence primitive that is able to combine mutliple nn layers

Nakroma added 3 commits May 28, 2024 10:52
This commit adds a Layer interface for the Python API. The Affine and ReLU classes are changed to extend this interface.
This commit fixes some small formatting issues in the modified classes.
This commit adds a Sequential primitive to the nn Python API. It is able to combine multiple nn layers into one sequential module.
@Nakroma Nakroma marked this pull request as ready for review June 17, 2024 11:12
@Baunsgaard
Copy link
Contributor

Hi @Nakroma

The code looks great! The PR is nearly done.

Currently missing

  • is backward test
  • and i am worried about multi return statements, where some layers return multiple variables. Would they work in the sequences?

best regards
Sebastian

Nakroma added 2 commits June 18, 2024 18:20
This commit adds the backwards pass to the Sequential primitives. It also makes a minor fix in the python MultiReturn so outputs of the instance can be properly accessed.
This commit lets the Sequential primitive correctly handle MultiReturns in the forward pass.
@Nakroma
Copy link
Contributor Author

Nakroma commented Jun 18, 2024

Hey @Baunsgaard

I actually completely forgot the backwards pass - sorry about that, it is added now!

About the multi returns - I wasn't 100% sure if returns for forward/backward passes are standardized. So I assumed here how it's handled in affine.py and relu.py is the standard - i.e. I check for a MultiReturn or a single node and return based on that. This might have to be changed depending on how the layer generator will work in the future though. Let me know if you think there is a better solution for that also.

Best,
Tarek

@Baunsgaard
Copy link
Contributor

Great, i like the change. seems like you found a few bugs in the process.

Can we add a few more tests, where

  1. a multi return layer goes into a multi return layer,
  2. a TestLayerImp goes to Multi to multi

and a few more variations.

Also can we verify that the layers get updated correctly on the backwards pass.

Once these are added I would consider the PR finished and ready for merging.

This commit adds more variations to Sequential testing involving MultiReturns. It also adds a test if the input gradient is set correctly on the backwards pass and fixes a bug where this was not the case on the affine layer.
@Nakroma
Copy link
Contributor Author

Nakroma commented Jun 30, 2024

@Baunsgaard just pushed a commit that addresses more variations and the layer update during backwards pass.

@Baunsgaard
Copy link
Contributor

@Baunsgaard just pushed a commit that addresses more variations and the layer update during backwards pass.

Great. I will take a look tomorrow!

This commit adds testing to verify that the layer gets updated correctly during forward and backward pass.
@Baunsgaard Baunsgaard closed this in 8e1e53b Jul 1, 2024
@Baunsgaard
Copy link
Contributor

@Nakroma
Thank you very much for the PR. it is now merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants