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.

Sign up for GitHub

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

Jump to bottom

Feat/tflog callback #290

Merged
merged 37 commits into from
Oct 18, 2024
Merged

Feat/tflog callback #290

merged 37 commits into from
Oct 18, 2024

Conversation

cxzhang4
Copy link
Collaborator

@cxzhang4 cxzhang4 commented Oct 11, 2024 •
edited
Loading

  • Update NEWS.md
  • Refactor logging functionality to avoid repeating code
  • Update style (period at the beginning of private functions)
  • Remove code that uses new R features
  • Refactor to use map_lgl() and avoid extraneous function calls
  • Remove magic numbers
  • Remove extraneous comments
  • Add documentation describing how to use TensorBoard
  • Log the current epoch as the step index
  • add a parameter that determines whether we log the individual training losses (sometimes only want validation)
  • Factor out the helper function for counting events

cxzhang4
cxzhang4 commented Oct 11, 2024
View reviewed changes
R/CallbackSetTB.R Outdated Show resolved Hide resolved
cxzhang4
cxzhang4 commented Oct 11, 2024
View reviewed changes
R/CallbackSetTB.R Outdated Show resolved Hide resolved
cxzhang4
cxzhang4 commented Oct 11, 2024
View reviewed changes
R/CallbackSetTB.R Outdated Show resolved Hide resolved
cxzhang4
cxzhang4 commented Oct 11, 2024
View reviewed changes
R/CallbackSetTB.R Outdated Show resolved Hide resolved
cxzhang4
cxzhang4 commented Oct 11, 2024
View reviewed changes
R/CallbackSetTB.R Outdated
#' Meaningful changes happen at the end of each epoch.
#' Notably NOT on_batch_valid_end, since there are no gradient steps between validation batches,
#' and therefore differences are due to randomness
# TODO: display the appropriate x axis with its label in TensorBoard
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use the step argument in log_event calls

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's ambiguous here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only did some basic sanity checking (basically just looked at the test case in TensorBoard), so wasn't 100% confident in the correctness at the time of writing. But I think there's not a whole lot to mess up here

cxzhang4
cxzhang4 commented Oct 11, 2024
View reviewed changes
Copy link
Collaborator Author

@cxzhang4 cxzhang4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See individual comments

@cxzhang4 cxzhang4 marked this pull request as draft October 11, 2024 10:00
@cxzhang4
all tests pass, I think this is parity with the previous broken commi…
7c9f431 
…t. still need to incorporate the step logging
@cxzhang4 cxzhang4 marked this pull request as ready for review October 11, 2024 16:41
@cxzhang4
Copy link
Collaborator Author

The conversations left open are the ones that I see as slightly more ambiguous (we might want to discuss design, or look more closely to confirm that they have truly been addressed)

cxzhang4
cxzhang4 commented Oct 11, 2024
View reviewed changes
NEWS.md Outdated Show resolved Hide resolved
sebffischer
sebffischer requested changes Oct 15, 2024
View reviewed changes
Copy link
Member

@sebffischer sebffischer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good!

Mostly minor style issues that are not so important in the tests but it's good to generally keep them in mind.

R/CallbackSetTB.R Outdated Show resolved Hide resolved
R/CallbackSetTB.R Show resolved Hide resolved
R/CallbackSetTB.R Outdated Show resolved Hide resolved
R/CallbackSetTB.R Outdated Show resolved Hide resolved
R/CallbackSetTB.R Outdated Show resolved Hide resolved
tests/testthat/test_CallbackSetTB.R Outdated

events = mlr3misc::map(collect_events(pth0)$summary, unlist)

n_train_loss_events = sum(unlist(mlr3misc::map(events, event_tag_is, tag_name = "train.loss")))
Copy link
Member

@sebffischer sebffischer Oct 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

again map_lgl

tests/testthat/test_CallbackSetTB.R Show resolved Hide resolved
tests/testthat/test_CallbackSetTB.R Outdated Show resolved Hide resolved
tests/testthat/test_CallbackSetTB.R Outdated Show resolved Hide resolved
tests/testthat/test_CallbackSetTB.R Outdated

events = mlr3misc::map(collect_events(pth0)$summary, unlist)

n_train_loss_events = sum(unlist(mlr3misc::map(events, event_tag_is, tag_name = "train.loss")))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use map_lgl

cxzhang4 and others added 15 commits October 15, 2024 18:33
@cxzhang4 @sebffischer
logical -> flag, since the length of this arg must be 1
74757a7 
Co-authored-by: Sebastian Fischer <[email protected]>
@cxzhang4 @sebffischer
"TensorBoard events" appears to be a more idiomatic phrase
72d23f4 
Co-authored-by: Sebastian Fischer <[email protected]>
@cxzhang4 @sebffischer
map() -> walk(), since we don't use the return value of map()
beaca43 
Co-authored-by: Sebastian Fischer <[email protected]>
@cxzhang4 @sebffischer
map() -> walk(), since we don't use the return value of map()
1bf2939 
Co-authored-by: Sebastian Fischer <[email protected]>
@cxzhang4 @sebffischer
Apply suggestions from code review
03aad62 
setnames() is cleaner, allows us to create a list and set the names in a single line

Co-authored-by: Sebastian Fischer <[email protected]>
@cxzhang4 @sebffischer
add a default value for the log_train_loss param
e209092 
Co-authored-by: Sebastian Fischer <[email protected]>
@cxzhang4 @sebffischer
add package dependency
694ea85 
Co-authored-by: Sebastian Fischer <[email protected]>
@cxzhang4 @sebffischer
" Now that we have a default no need to specify log_train_loss"
a8f741d 
Co-authored-by: Sebastian Fischer <[email protected]>
@cxzhang4 @sebffischer
better test description
795b3f1 
Co-authored-by: Sebastian Fischer <[email protected]>
@cxzhang4 @sebffischer
Apply suggestions from code review
6b96aed 
Make test more efficient

Co-authored-by: Sebastian Fischer <[email protected]>
@cxzhang4
unlist(map -> map_lgl
2113faf
@cxzhang4
removed library import in test file, added tfevents:: when we use a f…
afbb074 
…unction from tfevents
@cxzhang4
removed extra TODO, don't bind to variables when the value is only us…
074f73b 
…ed once
@cxzhang4
remove more unnecessary vars, increase batch size to make tests run f…
5f7d56f 
…aster
@cxzhang4
access path field of callback, ifelse -> if
a1ddb7d
@sebffischer sebffischer merged commit b4594cc into main Oct 18, 2024
6 checks passed
@sebffischer sebffischer deleted the feat/tflog-callback branch October 18, 2024 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sebffischer sebffischer sebffischer requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@cxzhang4 @sebffischer