-
Notifications
You must be signed in to change notification settings - Fork 0
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
DM-34264: Design Task for applying real-bogus scoring #4
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a couple of comments. I tried to limit them to naming, docstrings, etc., and only on the preexisting files.
""" Constructor | ||
""" | ||
super(dummyClassifier, self).__init__() | ||
super().__init__() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why remove the docstring? You wanted to suggest corrections?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No docstrings on __init__
methods, and docstrings should contain information you wouldn't otherwise get from the method name.
https://developer.lsst.io/python/numpydoc.html#placement-of-class-docstrings
@@ -21,9 +19,6 @@ def __init__(self): | |||
self.fc2 = nn.Linear(128, 1) | |||
|
|||
def forward(self, x): | |||
""" Forward pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why remove the docstring? You wanted to suggest corrections?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This docstring doesn't contain any real information. The method docstring in the parent class is more informative, so we should let the system default to that.
https://pytorch.org/docs/stable/generated/torch.nn.Module.html
The interface will need to be fleshed out and have an actual test suite to go with it (the current test will fail); this provides an API for the Task to code against.
1814c31
to
80bf359
Compare
I've responed to your comments and rebased everything. Please take a look at the rest of the task as well. |
Looks good. I'll start testing the functionality after this is merged, and capture potential needed changes along the way. |
DM-34264: Design Task for applying real-bogus scoring
This implements a working Task (tested with a mock of
interface.inter
), and what should be a viable API for the model interface.The weights loading is currently a hack, using a config field. We won't want that in production, but until we have a better sense of the size of the weights file (both on disk and in-memory), we won't know whether we will need this to be in a separate process (e.g. a REST API server somewhere else), or how we will keep the pytorch model instantiated in memory between jobs. This at least should be enough to let us start testing real/bogus in a working pipeline.