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

Update plot rendering from textplots-rs to termplot #485

Closed
wants to merge 6 commits into from

Conversation

jklott
Copy link
Contributor

@jklott jklott commented Jul 9, 2023

Pull Request Template

Checklist

  • Confirm that run-checks.sh has been executed.

Related Issues/PRs

This solves issue here: #211

Changes

Rendering of plots relied on textplot-rs which has a couple of issues:

  1. The dots were not rendering correctly, causing a confusion in what the graph is showing.
  2. textplot-rts has a dependency with nom which causes a compilation warning when building burn

The changes made reflect the requirements for using termplot. The main two changes made:

  1. Coloring is not an option for termplot so all references to color have been removed.
  2. termplot uses f64 values instead of f32 values which were previously used. Conversions have been made where necessary.
  3. width variable was of type u32 but has been changed to usize to match height. Use of usize is better practice in this case as well.
  4. Syntax has been modified as needed to work with termplot.

Testing

Testing has been done by rendering plots for training data such as mnist example.

@jklott
Copy link
Contributor Author

jklott commented Jul 9, 2023

Attached is an example rendering on my system which uses VSCode dark mode on Linux Mint.

improved_rendering

@antimora
Copy link
Collaborator

antimora commented Jul 10, 2023

I can confirm no compilation warning.

I have run your changes locally on Mac M1 via vscode's term, the dots still appear as before. So your term must have a nicer fonts that render the dots differently. Maybe there is a setting in the library to adjust this somehow so an average user can see the plots clearly.

Here is what I am seeing now: image

Also there are RED/BLUE labels in cli.rs that probably need to be updated since we don't have colors:

" - {} RED: train | BLUE: valid \n{}",

You mentioned on chat that you were going to try auto scaling for the plot, were you able to achieve it?

@antimora antimora requested a review from nathanielsimard July 10, 2023 03:40
Copy link
Member

@nathanielsimard nathanielsimard left a comment

Choose a reason for hiding this comment

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

It doesn't work with the text-accuracy example from my side, so maybe there is more work to be done before being merged.

Comment on lines +180 to +181
.set_x_label("X-Axis: Iterations")
.set_y_label("Y-Axis: Accuracy")
Copy link
Member

Choose a reason for hiding this comment

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

The Y-Axis isn't always accuracy, it depends on the metric.

@jklott
Copy link
Contributor Author

jklott commented Jul 12, 2023

Closing this PR as burn-rs is moving away from console based plotting and instead will be using a proper dashboard in the future.

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

Successfully merging this pull request may close these issues.

3 participants