Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
TrainingLoop: refactor progress printer and add CSVLogger #668
TrainingLoop: refactor progress printer and add CSVLogger #668
Changes from 4 commits
565fff8
7e2e063
6f51d43
6625371
9008b3a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
It seems very unlikely to me that we actually need to store
path
in addition tofoundationFile
. Consider whether it can/should be dropped because you can get it fromfoundationFile
.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.
Same. Removed foundationFile and kept the path.
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.
Hmm, I'm not sure you want to open and close the file for every line logged, though. (I am presuming that the
foundationFile
object keeps the file open)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.
How did you “resolve” this comment?
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 would any of these things be
nil
, and why are we bailing out when they'renil
? That should be explained in a comment. Ditto forstats
below. Also, why are these two separate guard statements?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.
Merged stats into the same guard statements.
These are designed as optionals in existing TrainingLoop. I think that's because it's not the must-have values to complete a training process. I added an inline comment.
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.
OK, first, I would rather see something like
because it describes the situation at a semantic level rather than at the level of what some code did.
(also writing "No-Op" adds nothing to what is already very obvious from the code)
But that said, it seems very unlikely that the comment I'd like to see is true of any but the last property. All the others refer to values that have nothing to do with logging. So I want to know what causes
epochIndex
to benil
, for example.It's a big design flaw in trainingLoop that it has so many optionals, and that makes this task more difficult, but I believe that's not the code you're working on(?)
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.
These variables were originally designed to be optionals to store temporary data in protocol: https://github.com/tensorflow/swift-models/blob/master/TrainingLoop/TrainingLoop.swift#L76
The generic TrainingLoop that implements the protocol does set all these optionals. My guess on why it was designed so is that it allows other TrainingLoops not setting them.
So the point on the comment is NOT "if stats logging doesn't request it then these properties will be nil", but "if these properties are nil No-op on the CSVLogger".
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.
Then you should delete the comment. The code very clearly says that all by itself, so the comment explains nothing.
I don't know if you're missing the point I'm trying to make, or you just disagree with it, but I'm doing this code review 100% for your benefit as a programmer. If the review process is blocking your progress, please feel free to just commit the changes, and decide separately about whether you want the feedback I'm giving you here. If you do, we can continue to discuss it.
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.
Doc comments are missing on this and the following method.
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.
length in what unit? I suppose it's probably characters. Being a top-level declaration this should have a doc comment, and that would be a perfect place to put the answer. Is this the number of
=
signs, or the whole length printed, or…?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 don't know what a "handler" is. That makes me suspect this class is not really representing any abstraction.
Since it has no storage, there's no reason to make it a
class
rather than astruct
, but I don't think it should exist as a type. This thing is just a generic function overTrainingLoopProtocol
instances.print
not a great name for the method, becausepp.print(myLoop, someEvent)
looks like it's printingmyLoop
andsomeEvent
, especially given the precedent set bySwift.print(myLoop, someEvent)
.A more appropriate idiom would be: