Closed BradyPlanden closed 1 month ago
Attention: Patch coverage is 99.48980%
with 1 line
in your changes missing coverage. Please review.
Project coverage is 99.09%. Comparing base (
c8b00e6
) to head (1315108
). Report is 3 commits behind head on develop.
Files with missing lines | Patch % | Lines |
---|---|---|
pybop/plot/voronoi.py | 99.07% | 1 Missing :warning: |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Thanks @NicolaCourtier, this should be ready for another review after the tests pass.
Any input on the method name? I'm torn between something simple like plot_surface
or more informative plot_voronoi2d
. Perhaps, surface
is more understandable for users.. For reference, Pints call their implementation surface
. I'll also replace more of the plot2d
calls in the examples with this once we are happy with the name, as it's faster and more representative of the cost landscape from the optimisation perspective.
Thanks, I completely agree with updating the default 2d plot to this one for both speed and accuracy. I'd be happy with either plot_surface
or plot_voronoi
/2d
, but then I think we should also update the name of plot2d
to distinguish between the two, perhaps this could become plot_contours
?
This now has a breaking change for the plotting methods as described in the changelog. Essentially, pybop.plot_XXX
becomes pybop.plot.XXX
where possible. This adds clearer separation for the plotting methods, and allows for better method naming. A few of the methods have a larger rename, such as, plot2d
becoming plot.contour
and plot_voronoi_2d
is now plot.surface
Description
pybop.plot.surface
)Optimiser.log
for consistent object typespybop.plot_XXX
methods becomepybop.plot.XXX
Plots
Here's the voronoi for a particle swarm parameterisation workflow. The voronoi works well for the heuristic / global optimisation algorithms, as they tend to explore the landscape.
For gradient based optimisers, the landscape is less informative. I believe this is intentionally helpful to the user, as it actually conveys the lack of information we know about the search space.
This PR should probably have a follow up that improves the documentation on how to use this plot, as it's not very common.
Issue reference
Fixes #361
Review
Before you mark your PR as ready for review, please ensure that you've considered the following:
Type of change
Key checklist:
$ pre-commit run
(or$ nox -s pre-commit
) (see CONTRIBUTING.md for how to set this up to run automatically when committing locally, in just two lines of code)$ nox -s tests
$ nox -s doctest
You can run integration tests, unit tests, and doctests together at once, using
$ nox -s quick
.Further checks:
Thank you for contributing to our project! Your efforts help us to deliver great software.