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

Merged
merged 2 commits into from
Nov 21, 2024
Merged

Avoid introducing MPC.plot #120

merged 2 commits into from
Nov 21, 2024

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-commenter commented Oct 17, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 4 lines in your changes missing coverage. Please review.

Project coverage is 98.93%. Comparing base (3833301) to head (071f134).
Report is 10 commits behind head on main.

Files with missing lines Patch % Lines
src/plot_sim.jl 50.00% 4 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.


🚨 Try these New Features:

@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

@franckgaga
Copy link
Member

see https://discourse.julialang.org/t/documenting-a-plot-recipe/122423/3

that's the best compromise I found to avoid the confusion.

@franckgaga franckgaga merged commit 1a451a9 into main Nov 21, 2024
4 checks passed
@franckgaga franckgaga deleted the fixplot branch November 21, 2024 16:18
@franckgaga franckgaga mentioned this pull request Nov 21, 2024
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