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

DesignCreator should inherit from torch.nn.Module #369

Closed
julianhoever opened this issue Aug 13, 2024 · 1 comment · Fixed by #376
Closed

DesignCreator should inherit from torch.nn.Module #369

julianhoever opened this issue Aug 13, 2024 · 1 comment · Fixed by #376
Labels
bug Something isn't working

Comments

@julianhoever
Copy link
Contributor

The abstract class DesignCreator should also inherit from torch.nn.Module because it is used to ensure that a torch.nn.Module always have a create_design function for translation. Therefore it is mostly used in combination with a torch.nn.Module.

class DesignCreator(ABC):
@abstractmethod
def create_design(self, name: str) -> Design: ...

In addition in the Sequential class we assume that submodules of type DesignCreator passed to the constructor are also torch.nn.Module, because we cast them to torch.nn.Module which is not possible if the submodules of type DesignCreator not inherit from torch.nn.Module.

class Sequential(DesignCreator, torch.nn.Sequential):
def __init__(self, *submodules: DesignCreator):
super().__init__(*cast(tuple[torch.nn.Module, ...], submodules))
def create_design(self, name: str) -> Design:
registry = _Registry()
submodules: list[DesignCreator] = [
cast(DesignCreator, m) for m in self.children()
]
for module in submodules:
registry.register(module.__class__.__name__.lower(), module)
subdesigns = list(registry.build_designs())
return _SequentialDesign(
sub_designs=subdesigns,
name=name,
)

Perhaps the name DesignCreator is not a good choice.

@julianhoever julianhoever added the bug Something isn't working label Aug 13, 2024
@julianhoever julianhoever changed the title DesignCreator should inherit from torch.nn.Module DesignCreator should inherit from torch.nn.Module Aug 13, 2024
@glencoe
Copy link
Contributor

glencoe commented Aug 13, 2024

I'm not sure i agree. For testing purposes and modularity, i like the two to be separated. But i agree that adding Module there would communicate intent better. What do you think about introducing a DesignCreatorModule or just CreatorModule (that users should inherit from) and keep the DesignCreator as is?

@julianhoever julianhoever linked a pull request Aug 21, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants