Skip to content

Conversation

@kevinchern
Copy link
Collaborator

No description provided.

@kevinchern kevinchern requested a review from thisac October 21, 2025 00:28
Copy link
Contributor

@thisac thisac left a comment

Choose a reason for hiding this comment

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

Have yet to look at tests. Would this fit better in an nn folder, allowing more torch modules to be further organized into their own files. Can still be kept under the torch.nn namespace.

@VolodyaCO VolodyaCO self-requested a review October 21, 2025 21:14
Copy link
Collaborator

@VolodyaCO VolodyaCO left a comment

Choose a reason for hiding this comment

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

Left a question about SkipLinear

Copy link
Collaborator

@anahitamansouri anahitamansouri left a comment

Choose a reason for hiding this comment

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

Thanks very much Kevin for the great work. I have left my comments.

@kevinchern
Copy link
Collaborator Author

RE @thisac : restructured the file into (sub)modules by pattern-matching pytorch's structure.
RE @VolodyaCO : configs are now stored recursely for nested models :D. reference for resnet added in the docstring
RE @anahitamansouri : fixed typos and added reference for resnet in docstring

Copy link
Collaborator

@anahitamansouri anahitamansouri left a comment

Choose a reason for hiding this comment

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

Thanks very much Kevin for addressing the comments.

Copy link
Contributor

@thisac thisac left a comment

Choose a reason for hiding this comment

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

Thanks @kevinchern. I feel like the tests/utils and its tests could be somewhat simplified, and maybe cut down a bit, otherwise nothing major.

@kevinchern
Copy link
Collaborator Author

RE @thisac : addressed most comments and left some clarifying questions in comments

@kevinchern
Copy link
Collaborator Author

@thisac updated again based on your feedback on best practices. Thanks!!

  1. added more tests for the store_config case and separated them as recommended
  2. split linear tests as recommended

from tests import helper_functions


class TestUtils(unittest.TestCase):
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
class TestUtils(unittest.TestCase):
class TestHelperFunctions(unittest.TestCase):

)

def forward(self, x: torch.Tensor) -> torch.Tensor:
"""Transforms the input `x` with the modules.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"""Transforms the input `x` with the modules.
"""Transforms the input ``x`` with the modules.

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.

5 participants