-
Notifications
You must be signed in to change notification settings - Fork 27
Conversation
Pull Request Test Coverage Report for Build 1611058010
💛 - Coveralls |
deeprank/learn/NeuralNet.py
Outdated
try: | ||
self.hit_cutoff = self.state['hit_cutoff'] | ||
except Exception: | ||
print('No hit_cutoff provided') |
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.
Please use logger
to output messages.
I guess this Exception
only applies to our published models. If so, we have to tell users what to do in the message, like No "hit_cutoff" found in {model}. Please set it in function "test()" when doing prediction"
.
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.
The hit cutoff is used only if target values are provided. In prediction mode you do not need it, you need it for benchmark only.
deeprank/learn/NeuralNet.py
Outdated
@@ -766,7 +770,8 @@ def _train(self, index_train, index_valid, index_test, | |||
return torch.cat([param.data.view(-1) | |||
for param in self.net.parameters()], 0) | |||
|
|||
def _epoch(self, data_loader, train_model): | |||
|
|||
def _epoch(self, data_loader, train_model, test_model=False): |
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.
The name test_model
is misleading. It's acutally a flag to tell whether there is target
values or not in the data. So I suggest using e.g. has_target=True
?
Also, please add the docstring for new parameters.
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.
Very good suggestion, thanks !
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.
Thanks @manonreau I left a few comments for you to check
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.
Thanks Liang for the review, I modified the code accordingly.
deeprank/learn/NeuralNet.py
Outdated
try: | ||
self.hit_cutoff = self.state['hit_cutoff'] | ||
except Exception: | ||
print('No hit_cutoff provided') |
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.
The hit cutoff is used only if target values are provided. In prediction mode you do not need it, you need it for benchmark only.
deeprank/learn/NeuralNet.py
Outdated
@@ -766,7 +770,8 @@ def _train(self, index_train, index_valid, index_test, | |||
return torch.cat([param.data.view(-1) | |||
for param in self.net.parameters()], 0) | |||
|
|||
def _epoch(self, data_loader, train_model): | |||
|
|||
def _epoch(self, data_loader, train_model, test_model=False): |
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.
Very good suggestion, thanks !
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.
Thanks @manonreau , the changes look good!
One reminder for you is that we don't use print
but logger
for outputing message in DeepRank :-)
Add examples of code to run the CNN models from the paper on new data:
Example with 3DeepFace
Example with the docking scoring model
Changed NeuralNet and DataSet to handle test set with no target value