Skip to content
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 plotting capabilities and dataset restructure #178

Merged
merged 85 commits into from
Mar 21, 2024

Conversation

NicolaCourtier
Copy link
Member

@NicolaCourtier NicolaCourtier commented Jan 29, 2024

  • Update plotting, descriptions and examples - the same plots are implemented using a more flexible design based on StandardPlot - for example, layout keyword arguments can be passed rather than individual properties such as xaxis_title
  • Add getitem and check to Dataset class
  • Add plot_dataset

@NicolaCourtier NicolaCourtier linked an issue Jan 29, 2024 that may be closed by this pull request
Copy link

codecov bot commented Jan 29, 2024

Codecov Report

Attention: Patch coverage is 95.97701% with 21 lines in your changes are missing coverage. Please review.

Project coverage is 95.13%. Comparing base (2013161) to head (e513c33).
Report is 6 commits behind head on develop.

Files Patch % Lines
pybop/costs/_likelihoods.py 91.22% 5 Missing ⚠️
pybop/plotting/quick_plot.py 95.68% 5 Missing ⚠️
pybop/costs/fitting_costs.py 93.47% 3 Missing ⚠️
pybop/observers/observer.py 90.00% 3 Missing ⚠️
pybop/models/lithium_ion/echem_base.py 50.00% 2 Missing ⚠️
pybop/costs/design_costs.py 95.23% 1 Missing ⚠️
pybop/plotting/plot_problem.py 97.22% 1 Missing ⚠️
pybop/plotting/plotly_manager.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #178      +/-   ##
===========================================
+ Coverage    94.29%   95.13%   +0.83%     
===========================================
  Files           33       36       +3     
  Lines         1753     1994     +241     
===========================================
+ Hits          1653     1897     +244     
+ Misses         100       97       -3     

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

@BradyPlanden
Copy link
Member

BradyPlanden commented Feb 9, 2024

Hi Nicola,

Thanks for the addition! Unfortunately I'm a bit stuck as I'm not sure how to proceed with the current state of the diffs. I don't think we'll be able to catch differences in the base vs. head due to the full file deltas. I'm not keen on manually diffing them, maybe we can remove the filename changes and add them after the PR is approved.

Could you also include details of the codebase changes in the top PR comment? It would both help me and give us a bread trail for changes to these files.

@NicolaCourtier NicolaCourtier changed the title Update plotting capabilities Update plotting capabilities and dataset restructure Mar 20, 2024
Copy link
Member

@BradyPlanden BradyPlanden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent – thanks @NicolaCourtier! Glad to get this one finished, I've added a few suggestions for you to consider. Happy for this to be merged once you are happy.

As discussed, it would be good to have a normal merge on this to distiguish between the three PR's for debugging in the future.



def plot2d(
cost_or_optim, gradient=False, bounds=None, steps=10, show=True, **layout_kwargs
Copy link
Member

@BradyPlanden BradyPlanden Mar 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something like this could be a bit more clear. Could be helpful with generality in the future as well.

Suggested change
cost_or_optim, gradient=False, bounds=None, steps=10, show=True, **layout_kwargs
plot_object, gradient=False, bounds=None, steps=10, show=True, **layout_kwargs

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's update this when we allow for more general inputs in the future.


Parameters
----------
cost_or_optim : a callable cost function, pybop Cost or Optimisation object
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
cost_or_optim : a callable cost function, pybop Cost or Optimisation object
plot_object : a callable object that returns a scalar value. Example: pybop.cost or pybop.optimisation

"""

# Assign input as a cost or optimisation object
if isinstance(cost_or_optim, pybop.Optimisation):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if isinstance(cost_or_optim, pybop.Optimisation):
if isinstance(plot_object, pybop.Optimisation):


# Assign input as a cost or optimisation object
if isinstance(cost_or_optim, pybop.Optimisation):
optim = cost_or_optim
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
optim = cost_or_optim
optim = plot_object

plot_optim = True
cost = optim.cost
else:
cost = cost_or_optim
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
cost = cost_or_optim
cost = plot_object

tests/unit/test_plots.py Outdated Show resolved Hide resolved
NicolaCourtier and others added 2 commits March 21, 2024 09:59
Co-authored-by: Brady Planden <55357039+BradyPlanden@users.noreply.github.com>
@NicolaCourtier NicolaCourtier merged commit b2afea4 into develop Mar 21, 2024
31 checks passed
@NicolaCourtier NicolaCourtier deleted the 177-plotting-capabilities branch April 9, 2024 10:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update quick_plot for varying trace sizes Increase plotting capabilities
2 participants