-
Notifications
You must be signed in to change notification settings - Fork 24
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update optim.log to store cost #335
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #335 +/- ##
===========================================
+ Coverage 97.33% 97.47% +0.13%
===========================================
Files 42 42
Lines 2404 2418 +14
===========================================
+ Hits 2340 2357 +17
+ Misses 64 61 -3 ☔ View full report in Codecov by Sentry. |
Merge after #334 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Nicola! One area that needs a bit of attention, otherwise a nice addition :)
pybop/plotting/plot2d.py
Outdated
@@ -70,6 +70,21 @@ def plot2d( | |||
for j, yj in enumerate(y): | |||
costs[j, i] = cost(np.array([xi, yj])) | |||
|
|||
if plot_optim: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the aim of this codeblock to append the locations the optimiser visited to the cost landscapes? At the moment, it breaks the gradient plots below it due to cost
function being overwritten as a np.array
, and as such isn't used in the plot2d object.
I added the below to line 127 of unit/test_plots.py
to catch the error.
# Plot gradient cost landscape
pybop.plot2d(optim, gradient=True, steps=5)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great catch! Yes, it should have been costs
not cost
... and thanks for the test.
From further testing, I've seen that the extra points (and nans) can modify the contour plot in unexpected ways. So I have implemented interpolation to avoid the nans and made this feature optional with the switch use_optim_log
.
Co-authored-by: Brady Planden <55357039+BradyPlanden@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks @NicolaCourtier!
Description
Update the log attribute of the optimisation object to store the cost at each iteration as well as the parameter values. Use these cost values in the convergence plot instead of recomputing them.
Issue reference
Fixes #165
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.