RocketPy

RocketPy copied to clipboard

Pull request type

• [x] Code changes (bugfix, features)

Checklist

• [ ] Tests for the changes have been added (if needed)
• [ ] Docs have been reviewed and added / updated
• [x] Lint (black rocketpy/ tests/) has passed locally
• [x] All tests (pytest tests -m slow --runslow) have passed locally
• [x] 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

• [x] 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

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.

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!

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.

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

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 🙂

The PR is (again) ready for review as the requested changes have been implemented.

I look forward to your feedback

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

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.

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.

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.