CLI for plotting trajectories of binaries #5976
Conversation
| @show_or_save_plot_command() | ||
| @apply_stylesheet_command() | ||
| def plot_trajectories_command(h5_files, subfile_name_aha, subfile_name_ahb): |
There was a problem hiding this comment.
This looks great! The image that this produces is really cool!
Could you also add some tests for this? Similar to the test in tests/Unit/Visualization/Python/Test_PlotSizeControl.py
| return None, None | ||
|
|
||
|
|
||
| def plot_trajectory(AhA, AhB, sample_rate=15): |
There was a problem hiding this comment.
Please add documentation to this function that describes what the function does and also describes each argument. You find examples in the other Python files in this directory.
There was a problem hiding this comment.
Here is my revised version, with all the current comments/suggestions incorporated. I made the code more compact. The suggested as well as new options are documented in the help file. I'll keep working on the Test script while I wait for more feedback.
There was a problem hiding this comment.
Only a test is missing now as @knelli2 mentioned above. Maybe write some made-up data to a file in a setUp function, then run the CLI command to plot it (@knelli2 pointed to some examples). You can't really test that the plot looks correct, but at least this test makes sure that the code runs without error, and keeps working in the future.
You can squash everything into 1 commit.
| @@ -12,115 +14,110 @@ | |||
| from spectre.Informer import unit_test_build_path, unit_test_src_path | |||
| from spectre.Visualization.PlotTrajectories import plot_trajectories_command | |||
|
|
|||
| # Configure logging | |||
| logging.basicConfig(level=logging.INFO) | |||
There was a problem hiding this comment.
Move this to __main__ at the bottom, like this:
if __name__ == "__main__":
logging.basicConfig(level=logging.DEBUG)
unittest.main(verbosity=2)fixup for PlotTrajectories CLI fixup for Test of PlotTrajectories CLI fixup 2 for PlotTrajectories CLI fixup 3 fixup 4
|
Yay nice job 👍 |
Proposed CLI for plotting trajectories
Code review checklist