ENH: Implement optional plot saving

[nalquas]

Pull request type

• Code changes (bugfix, features)

Checklist

• Tests for the changes have been added (if needed)
• Docs have been reviewed and added / updated
• Lint (black rocketpy/ tests/) has passed locally
• All tests (pytest tests -m slow --runslow) have passed locally
• CHANGELOG.md has been updated (if relevant)

Current behavior

Currently, the draw functions responsible for most plots use matplotlib's show function. There is currently no (supported) way to automatically save the plots to disk. See #564

New behavior

The functions generating plots now have a filename parameter, by default None. If the filename is None, the plot is shown as before using matplotlib's show. If the filename is a file path string (e.g. "/absolute/path/to/file.png" or "relative/path/to/file.jpg"), the plot is saved to the specified file. The file ending in the given filename decides which format the plot will be saved in, with matplotlib supporting: eps, jpg, jpeg, pdf, pgf, png, ps, raw, rgba, svg, svgz, tif, tiff and webp.

Breaking change

• No (only added optional parameters)

Additional information

I did not modify functions calling several plot-drawing functions, e.g. functions such as all(...). The reason for this is that those are probably only used for manual testing/analysis, e.g. when using Jupyter Notebook. Users wanting automatic output probably prefer calling the individual plot functions directly as it allows for more fine-control. Additionally, this saves me from additional implementation effort 😅, and reduces complexity of this pull request. If this functionality is still wanted, this could be implemented in a future pull request.

Also, I may have missed some plotting functions. If you spot anything obvious missing, feel free to notify me and I will look into it.

I look forward to any feedback during code review.

Fixes #564

[Gui-FernandesBR]

Hey, thank you so much for your draft PR @nalquas , it looks good already.

I will take a look at it by the end of this week and see if I find any improvement opportunity.

Nice job.

[nalquas]

The PR is now ready for review. I did not modify the tests, though, and I am not sure about what needs to be done to get the docs updated.

Looking forward to your feedback!

[Gui-FernandesBR]

Absolutely amazing PR, we really appreciate your contribution. The fact all the tests are passing, including the slow tests, is already a good sign.

I have a few comments about it, we can discuss on top of them.

There are 2 other things I want/need to do before approving your PR: (1) build the documentation and verify it is working correctly, (2) test the getting started notebook one more time and verify the plots can be saved properly.

I understand your reason for not adding save options to the all_info methods. Maybe we should wait until someone requests it as a new feature before we actually implement it.

[Gui-FernandesBR]

I also would like to run code coverage on this PR, the workflow did not worked because the base branch is coming from a fork. I need to investigate it better.

[Gui-FernandesBR]

Btw @nalquas could you please run isort to sort the imports in the files you modified?

You could use this command: isort --profile black rocketpy/ tests/ docs/

You can run pip install isort if needed

[nalquas]

I have applied the suggestions, updated the docstrings, moved get_matplotlib_supported_file_endings into tools.py and run isort/black. I have also rebased the PR onto the current develop branch.

Feel free to review again 🙂

[Lucas-Prates]

Great PR, @nalquas ! Overall, it is looking very good. The one issue that might be troublesome is saving the plots from the Function class with non-default arguments. I believe it would require some non-trivial changes.

[nalquas]

The PR is (again) ready for review as the requested changes have been implemented.
I look forward to your feedback

Thank you @nalquas . The review will be much faster now. We are almost ready to merge this one.

Thanks for the contribution

Is there any remaining interest for this functionality from your side? There have been several hundred commits in your repo since then, so I would have to rebase and rewrite everything again.

[Gui-FernandesBR]

The PR is (again) ready for review as the requested changes have been implemented.
I look forward to your feedback

Thank you @nalquas . The review will be much faster now. We are almost ready to merge this one.
Thanks for the contribution

Is there any remaining interest for this functionality from your side? There have been several hundred commits in your repo since then, so I would have to rebase and rewrite everything again.

Actually @phmbressan has already rebased it but he mentioned he was not able to push to your branch since you are working on a fork. We had to move our development to other tasks.

I guess if you could check if you can give him permission to ush to your branch, it would be really nice.

We are totally interested in the feature, the problem is just that we prioritized other tasks in the past few months.

[nalquas]

I guess if you could check if you can give him permission to ush to your branch, it would be really nice.

Ah, that makes sense. I activated the "Allow edits by maintainers" checkbox now.

[codecov]

Codecov Report

Attention: Patch coverage is 83.05085% with 30 lines in your changes missing coverage. Please review.

Project coverage is 76.08%. Comparing base (a6a0f74) to head (28c2ed5).
Report is 11 commits behind head on develop.

Files with missing lines	Patch %	Lines
rocketpy/plots/environment_analysis_plots.py	42.85%	16 Missing ⚠️
rocketpy/mathutils/function.py	75.00%	2 Missing ⚠️
rocketpy/plots/plot_helpers.py	91.66%	2 Missing ⚠️
rocketpy/motors/hybrid_motor.py	50.00%	1 Missing ⚠️
rocketpy/motors/liquid_motor.py	50.00%	1 Missing ⚠️
rocketpy/motors/solid_motor.py	50.00%	1 Missing ⚠️
rocketpy/motors/tank.py	50.00%	1 Missing ⚠️
rocketpy/plots/aero_surface_plots.py	93.33%	1 Missing ⚠️
rocketpy/plots/environment_plots.py	88.88%	1 Missing ⚠️
rocketpy/plots/tank_plots.py	66.66%	1 Missing ⚠️
... and 3 more

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #597      +/-   ##
===========================================
+ Coverage    75.95%   76.08%   +0.12%     
===========================================
  Files           99       95       -4     
  Lines        11237    10997     -240     
===========================================
- Hits          8535     8367     -168     
+ Misses        2702     2630      -72     

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

[Gui-FernandesBR]

Partial review only. I'd like more time to test the feature during the weekend.

Nice job here, I think we are very close to merge this one.

[Gui-FernandesBR]

Adding a validation here would be nice:

I mean, a more specific message should help users. Something like "could not understand the file format, please check ..."

[Gui-FernandesBR]

@phmbressan something I also missed is an example of usage. We could do something really simple, maybe by the end of the "first simulation" section.

[phmbressan]

Thanks for your comments @Gui-FernandesBR . I have added a Exception message if the file format is unsupported and a small documentation section for the file saving. Should you have any other comments, don't hesitate.

[Gui-FernandesBR]

Really nice work, I believe rocketpy users will benefit a lot from this new feature!

Thank you, @nalquas @phmbressan !!