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

#237 - Clean up plot_recipes #1400

Merged
merged 3 commits into from
May 30, 2019
Merged

#237 - Clean up plot_recipes #1400

merged 3 commits into from
May 30, 2019

Conversation

schillic
Copy link
Member

See #237.

Most important changes:

  • Plotting a vector of sets now falls back to the plot recipes for each individual set. This saves lots of code duplication and is generally preferable for both efficiency and consistency. In particular, this allows to plot vectors containing lazy Intersections.
  • Arbitrary 1D sets can now be plotted.
  • Common plotting options are unified.

@schillic schillic requested a review from mforets May 25, 2019 16:12
Copy link
Member

@mforets mforets left a comment

Choose a reason for hiding this comment

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

Awesome 💯

I have two general comments to do as follow-up:

  • Add a way to easily change the defaults (idea: have a mutable wrapper type for the plotting defaults). This use cases arises often eg. if i want to define the aspectratio (unfortunately passing aspectratio = :auto is broken but that is another issue).
  • Now that it is easy to choose between plotting using directions or refinement, "relax" the condition in plot_list by letting the user decide which method to use. This allows to plot more sets, too (such as the lazy linear map of an intersection, which cannot be plotted ATM) -- this can also be fixed if we use support function instead of support vector in the iterative refinement algorithm, but still, offering both approaches for plotting seems natural.

Changes requested to handle the plots of bounded polyhedra (see comment below).

docs/src/lib/interfaces.md Outdated Show resolved Hide resolved
src/plot_recipes.jl Outdated Show resolved Hide resolved
src/plot_recipes.jl Outdated Show resolved Hide resolved
@schillic schillic requested a review from mforets May 27, 2019 18:01
@schillic
Copy link
Member Author

Sorry, I added the changes to the old commit, but they got a bit tangled with a rebase with master. Here is the new file.

@schillic schillic force-pushed the schillic/237 branch 2 times, most recently from a34e8fc to 4976871 Compare May 28, 2019 18:43
@schillic
Copy link
Member Author

Wow, building time of the docs increased from <17 minutes to 28 minutes here. What is so different? I even reduced the total number of methods here...
There seems to be is something specific with our way to include docs that Documenter is terribly slow with.

@schillic
Copy link
Member Author

I finally understood now what is going on with the plotting.

The part that got terribly slow was the zonotope algorithm in the manual. There we plotted 100 resp. 200 sets, which normally is not too much, but here these sets are lazy and every set is the previous set inside another nesting level. I was actually surprised that this even worked. 100 sets take 30 seconds in master. 200 sets take 300 seconds in master.

In this branch I had introduced helper functions that are called from the loop for individual handling of each set in the list. Julia tries to be smart and compiles a new version for every new set (recall that we build nested lazy sets in that zonotope code, so each set is different type-wise) and this costs a lot (0.05 seconds every time, but this sums up). In master we hardcoded the overapproximation/vertex conversion inside the loop; hence julia cannot compile efficiently; this means that the second run is potentially slower (i guess in this application it does not matter), but also that there is less compilation overhead in the first run (which is way too big here).

Conclusion: I finally added the hardcoded version as an option (deactivated by default because I like the individual plot recipes more and in our Reachability applications the sets always have the same type so this won't happen (and anyway we have our own plot recipe in Reachability - which we maybe want to remove?)); when we use the zonotope algorithm with concrete operations (#1413), this problem will also vanish, but it is good to keep this fast option for other cases that might pop up.

Copy link
Member

@mforets mforets left a comment

Choose a reason for hiding this comment

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

Thanks for the summary.

@schillic schillic merged commit fe2132b into master May 30, 2019
@schillic schillic deleted the schillic/237 branch May 30, 2019 20:32
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.

2 participants