Skip to content

Conversation

@f0reachARR
Copy link
Contributor

Thank you for developing this awesome testing tool!

This PR proposes small improvements to better suit our use case.

Issue: Currently, the launch terminates automatically when MCAP playback finishes. We need to observe system behavior for some time after playback ends.

Changes:

  • Added ignore_playback_finish option to prevent automatic launch termination after MCAP playback completes
  • Fixed the run.default usage example in README.md (the previous example Clock Configuration caused errors)

* run.default with params now works written in README.md
* To meet our requirements, controlling shutdown condition in `Run` class is needed
@troygibb
Copy link
Contributor

Hey thanks so much for the contribution @f0reachARR!!

The structure here looks great, but I would like to see some testing coverage for this option. There are number of examples under ./test you can take some inspiration from. Also please update the README when you get a chance, I'm not seeing your helpful fix you mention in the PR description.

@troygibb
Copy link
Contributor

Also any chance you could describe your use case in more detail? How do you terminate the test?

@f0reachARR
Copy link
Contributor Author

Thank you for the advice!

I’ve added the tests and the README.

I would like to use replay_testing not only for sensing but also for simulations.
After launching the system, several inputs are provided via MCAP. Even after MCAP finishes, the simulation continues running for a while, and the simulator then shuts down automatically.
Once it has terminated, I use the analyze function to evaluate the validity of the time-series outputs.

@troygibb
Copy link
Contributor

troygibb commented Dec 4, 2025

amazing! thanks the README and tests look good. I'll merge once CI passes. Thanks so much for your contribution 😄

@troygibb
Copy link
Contributor

troygibb commented Dec 4, 2025

Looks like pre-commit is failing-- just some basic formatting changes you'll need to add by running pre-commit run --all-files

@f0reachARR
Copy link
Contributor Author

Sorry for mistakes! I just ran pre-commit on my changes!

@troygibb troygibb merged commit 8a2b2fd into polymathrobotics:main Dec 10, 2025
5 checks passed
@troygibb
Copy link
Contributor

troygibb commented Dec 10, 2025

@f0reachARR merged! Thanks so much for your contribution!

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