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

Spectator observer and convert existing observers to dedicated crates #311

Merged
merged 24 commits into from
Jan 10, 2024

Conversation

stefan-k
Copy link
Member

@stefan-k stefan-k commented Jan 6, 2023

A simple egui observer which shows the progress of the optimization in plots. Plots can selectively be shown or hidden.
This is still very basic and will change substantially until it is ready for merging.

Feedback is welcome!

Todos

  • Turn this into separate crate argmin-observer-egui
  • Put SlogLogger in dedicated crate argmin-observer-slog
  • Remove unnecessary slog_logger feature from argmin
  • Change all examples in argmin to use new slog logger crate
  • Rename WriteToFile to ParamWriter
  • Put ParamWriter in dedicated crate argmin-observer-paramwriter
  • Adapt argmin-observer-slog documentation
  • Adapt argmin-observer-paramwriter documentation
  • Docs on how to install spectator
  • Clean up spectator code

Caveats

  • Apparently GUIs need to run in the main thread, which is not possible in this case. I solved this by running the GUI in a separate process using the procspawn crate. Samples are then sent via ipc_channel which unfortunately doesn't work on Windows. Therefore Windows support is not possible until there is a reasonable way to communicate with the process that also works on Windows.
  • I haven't found a way to redraw the GUI only when new data comes in. Either it is redrawn as often as possible (with every call to update) or only when there is "GUI input" such as mouse movements. I'd like to redraw whenever there is new data on the channel.
  • This slows down the optimization substantially, which is probably fine and shouldn't be noticeable for cost functions that aren't super fast to compute. I guess such observers are used only during development/tuning and not "in production", where speed matters.

Changes outside of the new observer

  • get_float of KVType also works for all other numeric types now by converting those to floats. int and uint are cast to f64, therefore this operation may be lossy.
  • SlogLogger is now part of the crate argmin-observer-slog
  • WriteToFile was renamed to ParamWriter and is now part of the crate argmin-observer-paramwriter. The enum WriteToFileSerializer was renamed to ParamWriterFormat and the variant Bincode was renamed to Binary.
  • ParamWriter now chooses the extension based on the format. .json for JSON and .bin for Binary.
  • The Observe trait now has a observer_final method which is executed at the end of an optimization run.
  • observe_init in the Observe trait now has access to the optimization state

Other things for the future

  • sloglogger needs tests
  • paramwriter needs tests

@stefan-k stefan-k self-assigned this Jan 6, 2023
@stefan-k stefan-k force-pushed the egui_observer branch 2 times, most recently from 1a1b0b3 to d0744ee Compare February 3, 2023 10:13
@codecov-commenter
Copy link

codecov-commenter commented Feb 3, 2023

Codecov Report

Attention: 654 lines in your changes are missing coverage. Please review.

Comparison is base (220e0d5) 93.30% compared to head (31ccefc) 90.25%.

Files Patch % Lines
tools/spectator/src/app.rs 0.00% 286 Missing ⚠️
tools/spectator/src/connection.rs 0.00% 143 Missing ⚠️
observers/spectator/src/observer.rs 56.76% 99 Missing ⚠️
tools/spectator/src/data.rs 0.00% 64 Missing ⚠️
tools/spectator/src/main.rs 0.00% 30 Missing ⚠️
tools/spectator/src/telemetry.rs 0.00% 20 Missing ⚠️
tools/spectator/src/message.rs 0.00% 8 Missing ⚠️
argmin/src/core/kv.rs 91.42% 3 Missing ⚠️
observers/paramwriter/src/lib.rs 95.45% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #311      +/-   ##
==========================================
- Coverage   93.30%   90.25%   -3.05%     
==========================================
  Files         117      126       +9     
  Lines       18875    19679     +804     
==========================================
+ Hits        17611    17761     +150     
- Misses       1264     1918     +654     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@stefan-k stefan-k force-pushed the egui_observer branch 2 times, most recently from b64188a to ba8da6d Compare February 4, 2023 07:53
@stefan-k stefan-k force-pushed the egui_observer branch 2 times, most recently from 91f0c7f to 61850cc Compare February 26, 2023 10:34
@stefan-k stefan-k force-pushed the egui_observer branch 3 times, most recently from 4eeda1e to d28a203 Compare January 9, 2024 09:04
@stefan-k stefan-k marked this pull request as ready for review January 10, 2024 10:39
@stefan-k stefan-k force-pushed the egui_observer branch 4 times, most recently from 7142da0 to 27fbba1 Compare January 10, 2024 13:31
@stefan-k stefan-k changed the title Egui observer Spectator observer and convert existing observers to dedicated crates Jan 10, 2024
@stefan-k stefan-k force-pushed the egui_observer branch 2 times, most recently from 789086b to 17b517d Compare January 10, 2024 15:41
@stefan-k stefan-k merged commit 355a4a2 into argmin-rs:main Jan 10, 2024
16 checks passed
@stefan-k stefan-k deleted the egui_observer branch January 10, 2024 15:50
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.

2 participants