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.

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

Improving collect #30

Open
dfalbel opened this issue Jan 10, 2023 · 1 comment
Open

Improving collect #30

dfalbel opened this issue Jan 10, 2023 · 1 comment

Comments

@dfalbel
Copy link
Member

dfalbel commented Jan 10, 2023

Some minor notes:

  • I'm seeing this error:
> knitr::opts_chunk$set(
+   collapse = TRUE,
+   comment = "#>"
+ )
> library(tfevents)
> temp <- tempfile()
> set_default_logdir(temp)
> log_event(text = "hello world")
> for (i in 1:10) {
+   log_event(hello = log(i + runif(1)))
+ }
> events <- collect_events(temp)
> events
Error: Corrupt x: no names
summaries <- collect_events(temp, type = "summary")
summaries
  • Is this expected? Are summary objects meant to return themselves when subsetted?
summaries$summary[[1]][[1]][[1]][[1]][[1]]
  • Would it be possible to make as.list() return the values as R objects directly?
as.list(summaries$summary) # instead of sapply(,value)`
  • Is there anything else a user would do with a <tfevents_summary_values> other than extract the value? Maybe we don't need expose the protobuf structure and value() function and can just return R values to the user. E.g., collect_events() returns a data frame with a list column, each entry an atomic R vector, maybe with different class()'s for the different plugin types like structure(c(...), class = "tfevents_image", dim = c(128,3,3). What do you think?

con <- events_logdir(temp)
collect_events(con, n = 1, type = "scalar")

This is great!

Originally posted by @t-kalinowski in #27 (comment)

@dfalbel
Copy link
Member Author

dfalbel commented Jan 11, 2023

  • Fixed the error, it was actually a bug in the format implementation for tfevents_events.

  • Summaries are lists of summary_values vectors (as a summary can potentially contain many summary_values). The summary_values are vctrs, and they behave like R atomic vectors, ie subsetting them with [[ returns a new vector with that element. Accessing individual fields can be done with vctrs::field()

  • Yes, will implement as.list()

  • I taught about that but there are a few points:

    • There might be significant computation when casting from protobuf to an R object. Eg, Images are stored as binary PNG blobs and casting them to R objects requires us to read them with png::readPNG(). Same for audio files, etc.
    • Since plugin metadata could be arbitrary data, I think it's important to expose it entirely.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant