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
refactor: clean up GraphDataset and Trainer class #255
Conversation
deeprankcore/dataset.py
Outdated
if any(key in grp for key in ("internal_edge_index", "internal_edge_data")): | ||
warnings.warn( | ||
"""Internal edges are not supported anymore. | ||
You should probably prepare the hdf5 file | ||
with a more up to date version of this software.""", DeprecationWarning) | ||
"""Internal edges are not supported anymore.\n | ||
You should probably prepare hdf5 files with a more up to date version of this software.""", | ||
DeprecationWarning) |
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.
do we still need this warning or can it go?
If it stays, I'll reword the warning.
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.
We can leave it for now, just in case. This warning was placed by Coos for older hdf5 files.
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.
@cbaakman is this warning still necessary?
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.
Finally at the end of this :-) I have only really minor suggestions
|
||
self.complete_exporter = ConciseOutputExporter(self.output_dir) | ||
self.neuralnet = neuralnet |
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.
Recall that neuralnet
argument so far is a class, and here you're assigning it to an attribute without properly initializing it. I would suggest declaring here model
attribute = None, and using neuralnet
later on as in the original code. It's still possible to access the details of the neural network through model
attribute, without messing with the class itself.
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 is not clear to me. Maybe we can sit down and you explain what you mean?
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.
will be taken care of in #234
closes: #152 and #194
also partially addresses: #236 (the rest will move to #234)