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

Avoid introducing MPC.plot #120

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Avoid introducing MPC.plot #120

wants to merge 1 commit into from

Conversation

baggepinnen
Copy link
Member

This PR tries to solve #119

RecipesBase.plot is the function that is actually called when plotting a result object, ModelPredictiveControl.plot is a different function with the same name (in another namespace), causing some confusion

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 0% with 3 lines in your changes missing coverage. Please review.

Project coverage is 98.93%. Comparing base (3833301) to head (d195086).

Files with missing lines Patch % Lines
src/plot_sim.jl 0.00% 3 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #120   +/-   ##
=======================================
  Coverage   98.93%   98.93%           
=======================================
  Files          24       24           
  Lines        3575     3575           
=======================================
  Hits         3537     3537           
  Misses         38       38           

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

@franckgaga
Copy link
Member

franckgaga commented Oct 18, 2024

ModelPredicitiveControl.plot is not a real function. The sole purpose of this function is to document the keyword arguments of the plot recipes in the online documentation, and also by writing ?ModelPredictiveControl.plot in the REPL. But it does create some confusion. What's the best way to do that?

@franckgaga
Copy link
Member

@baggepinnen
Would it be a good idea to introduce three new method of controlplot and document these methods instead?

@baggepinnen
Copy link
Member Author

What's the best way to do that?

I'm not entirely sure, but I think that documenting RecipesBase.plot and inserting this docstring in the docs is the correct way to do it.

Would it be a good idea to introduce three new method of controlplot and document these methods instead?

Using RecipesBase is usually the most robust option in my experience, especially if you want the plot recipe to behave like an ordinary Plots.plot with all the features that bring.

@franckgaga
Copy link
Member

franckgaga commented Oct 22, 2024

Alright I need to change the md file to make the online doc compiles. I'll do that when I'm back from Japan. You can also try if you have time but these kind of stuff can wait IMO

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.

3 participants