JuliaReach / LazySets-JuliaCon21

LazySets.jl article for the JuliaCon 2021 Conference Proceedings
MIT License
5 stars 1 forks source link

Review 2 #5

Closed mforets closed 2 years ago

mforets commented 2 years ago

Reply to Review 2 (https://github.com/JuliaReach/LazySets-JuliaCon21/issues/4)

some tests are throwing warnings about polyhedra / LP solvers, is that expected behavior?

We are aware of these warnings, but we are not sure how to fix them.

Some references dont display DOIs (if they don't have any, maybe the Arxiv reference would be relevant?): 4, 11, 23. I didn't manage to find reference 21 with a quick Google search.

Thanks, we added links. Reference 21 is currently submitted and not public yet, but we plan to make it public by the end of the month.

It is probably due to the template, but copy-pasting code from the PDF adds unwanted spaces between every pair of characters.

True, seems like there is nothing we can do here.

Additional comments / legend / axis labels on Figure 1 would be welcome: a reader not familiar with reachability analysis might struggle to see the connection with Lotka-Volterra equations.

Good point. We added more explanations to the caption. We want to keep the figure simple in the introduction, so we prefer to not add axis labels. The colors do not have a meaning.

In the second code snippet, why is HalfSpace no longer displayed as a parametric type, as in HalfSpace{Float64,Vector{Float64}}?

We removed it to avoid ugly output. We now added a footnote. Let us know if you think that we should show the full output instead.

It would be nice to justify the omnipresence of linear programs: they are able to model a wide variety of real-life optimization problems

We added a small note and some references.

The previous sentence sounds like the half-space is part of the polytope somehow. Maybe add a comma after "orange"?

Thanks, we reformulated the sentence.

What is the complexity of these operations? They seem much more involved than membership checking.

This highly depends on the set representation. We added a small paragraph.

The distinction between lazy constructor and concrete function in Table 1 is not obvious at this stage of the paper: this would deserve an small explanation or a reference to what comes next.

We added a sentence to the caption.

Here it would help to repeat what MinkowskiSum is, as opposed to minkowski_sum, or refer to Table 1.

We reformulated the sentence.

Why the strange notation $E_+$? Why not $X$ and $Y$? If it has something to do with reachability applications, it would be nice to know just a little about the context.

We used "Y" now to avoid confusion.

We have also added a new reference (here) that reviews and implements such type of transformations using LazySets.

Could you say what this function is used for? From what I understand, its computation is one of the main selling points of the package, yet the reader unfamiliar with convex analysis may not see why it is so important

We added a sentence and moved a paragraph.

Why does $X$ have a different font than \texttt?

Thanks, fixed!

Wouldn't it make more sense to plot the vector $d$ as going from $(0, 0)$ to $(-3, 0.6)$?

We updated the plot. To avoid confusion, we now plot the distance instead.

Linear programs were introduced earlier but the acronym was not defined

We removed the acronym now.

You previously wrote this vector as $d = (-1.0, 1.0)^T$ (with decimals)

Right, fixed!

What does benchmarking demonstrate here?

We added a paragraph.

Again, it would really add a lot of value to list some of these applications

Alright, we added another example.

Underapproximations are not mentioned in what follows, are they not supported?

Yes, they are supported. We added a sentence at the very end of Section 4.

Keeping the polytope orange as before would be lovely if it were possible

Done. We used the default Julia colors, but you are right that it helps to use orange again.

Why isn't it possible in higher dimensions?

It is possible in principle but more complicated and not implemented yet (there is an open issue).

Types from Polyhedra.jl are used or mentioned several times without being formally introduced. A sentence about H vs V-representation may clarify things

We added some background in Section 2.1. Note that HPolytope/VPolytope etc. are LazySets types that wrap Polyhedra.jl types.

The 3D Makie plot is a bit too dark to see inside the volume

True. We managed to plot the edges of a triangulation with Makie. This looks much cleaner now.

(As a side note, the labels are not displayed using LaTeXStrings in Fig. 4 because Makie didn't support using that for the 3D plot (left)).

I think this may not be very clear to people unfamiliar with autodiff, especially the ForwardDiff.jl package. A motivated use of automatic differentiation would be more illuminating

We understand the concern, but since this is only a minor example and not an important application of LazySets, we prefer to keep this part short. We added a short explanation that ForwardDiff computes the gradient. We also added two references to substantiate the link between geometry and applications of automatic differentiation.

This sentence is unclear to me, and this may be why I struggle to understand the rest of the example

We reformulated the sentence and refer to Fig. 5. For this article, we try to avoid technical definitions, but if you think it is not understandable, we can be more specific.

Typo (no plural)

Fixed!

A legend highlighting the difference between $Z$ and $H$ on both sides would help

We added the legend in the caption and updated the plots.

Would it be possible to plot both vectors $d$ on Fig 5?

We tried out different plots but it became too crowded. So we decided to remove this code block altogether.

This formulation seems vague, I'm not sure I understand what you do there

We reformulated the sentence.

Such as?

We added a reference.

What does this mean?

We reformulated the sentence. It means that x(0) ∈ X where X is a set.

The legend is not quite enough for me to understand what is going on here. I struggle to see what happens in the case with non-deterministic inputs

We extended the explanation in the text and in the caption. With non-deterministic inputs your system has more possible behaviors, so the reachable states grow.

The following code snippet doesn't seem necessary to me

We removed it.

This kind of quantitative performance evaluation is very important, I would underline it more. The same goes for the ARCH-COMP win, if you can elaborate on that.

Thanks for this comment.

The ARCH-COMP proceedings should serve for the purpose of comparing the current status of JuliaReach implementations with respect to other implementations (usually MATLAB, C++, Python). The scalability of that tool has several sources, one of which is the efficient implementation in LazySets, but the algorithms themselves play an important role as well. In this article we mostly focused on the "sets" aspects and chose to not present the reachability algorithms (implemented in ReachabilityAnalysis.jl).

I find it strange that you wait until the conclusion to mention competing tools. I think at least a brief overview should come sooner, and most importantly you should say what these competitors are lacking in functionality, performance, documentation or whatever other aspects distinguishes LazySets from the pack. You could also add links to repos for these packages, as you did with the Julia ones.

The reason to discuss these libraries at the end is to have the necessary terminology. We swapped 6.1 and 6.2. The main difference to these packages is that they are written in other languages. We do not want to make hard claims here. We added the links as you suggested.

If the reader isn't familiar with multiple dispatch, this may not be clear. Give an example you encountered during development?

Good point, we added a cross reference to a previous example.

I haven't found the code for the examples in the paper repo, do you plan to add it soon?

The code is found in the file plots.jl in the img/ folder. Sorry, this was not clear. We now split up the file into smaller files to make the code easier to browse.

mforets commented 2 years ago

link to the new version: https://github.com/JuliaReach/LazySets-JuliaCon21/blob/679c946a3ca4fa868c4510431e7b0bbefc8c02d2/paper/paper.pdf

gdalle commented 2 years ago

We are aware of these warnings, but we are not sure how to fix them.

OK

Thanks, we added links. Reference 21 is currently submitted and not public yet, but we plan to make it public by the end of the month.

Thanks for the links / DOIs! References [4] and [16] are still missing one, maybe https://jmlr.org/papers/v18/16-107.html and https://arxiv.org/abs/2111.01454 would be fine ?

True, seems like there is nothing we can do here.

OK, maybe I'll mention it with the JuliaCon organizers to improve the LaTeX template.

Good point. We added more explanations to the caption. We want to keep the figure simple in the introduction, so we prefer to not add axis labels. The colors do not have a meaning.

OK. Very pretty plot btw!

We removed it to avoid ugly output. We now added a footnote. Let us know if you think that we should show the full output instead.

OK

We added a small note and some references.

OK

Thanks, we reformulated the sentence.

OK

This highly depends on the set representation. We added a small paragraph.

OK

We added a sentence to the caption.

OK

We reformulated the sentence.

OK

Why the strange notation $E_+$? Why not $X$ and $Y$? If it has something to do with reachability applications, it would be nice to know just a little about the context.

OK

We added a sentence and moved a paragraph.

OK

Thanks, fixed!

OK

We updated the plot. To avoid confusion, we now plot the distance instead.

OK

We removed the acronym now.

OK

Right, fixed!

OK

We added a paragraph.

OK

Alright, we added another example.

OK

Yes, they are supported. We added a sentence at the very end of Section 4.

OK

Done. We used the default Julia colors, but you are right that it helps to use orange again.

OK

It is possible in principle but more complicated and not implemented yet (there is an open issue).

OK, maybe add a footnote about this?

We added some background in Section 2.1. Note that HPolytope/VPolytope etc. are LazySets types that wrap Polyhedra.jl types.

OK

True. We managed to plot the edges of a triangulation with Makie. This looks much cleaner now. (As a side note, the labels are not displayed using LaTeXStrings in Fig. 4 because Makie didn't support using that for the 3D plot (left)).

OK

We understand the concern, but since this is only a minor example and not an important application of LazySets, we prefer to keep this part short. We added a short explanation that ForwardDiff computes the gradient. We also added two references to substantiate the link between geometry and applications of automatic differentiation.

OK

We reformulated the sentence and refer to Fig. 5. For this article, we try to avoid technical definitions, but if you think it is not understandable, we can be more specific.

OK

Fixed!

OK

We added the legend in the caption and updated the plots.

The blue is almost invisible, can you make it a little thicker or use a different linestyle?

We tried out different plots but it became too crowded. So we decided to remove this code block altogether.

OK

We reformulated the sentence.

OK

We added a reference.

OK

We reformulated the sentence. It means that x(0) ∈ X where X is a set.

OK

We extended the explanation in the text and in the caption. With non-deterministic inputs your system has more possible behaviors, so the reachable states grow.

Much better, maybe just define what a flowpipe is? When I first read I though it was some machine part in a helicopter ^^

We removed it.

OK

Thanks for this comment. The ARCH-COMP proceedings should serve for the purpose of comparing the current status of JuliaReach implementations with respect to other implementations (usually MATLAB, C++, Python). The scalability of that tool has several sources, one of which is the efficient implementation in LazySets, but the algorithms themselves play an important role as well. In this article we mostly focused on the "sets" aspects and chose to not present the reachability algorithms (implemented in ReachabilityAnalysis.jl).

OK

The reason to discuss these libraries at the end is to have the necessary terminology. We swapped 6.1 and 6.2. The main difference to these packages is that they are written in other languages. We do not want to make hard claims here. We added the links as you suggested.

Fine by me!

Good point, we added a cross reference to a previous example.

OK

The code is found in the file plots.jl in the img/ folder. Sorry, this was not clear. We now split up the file into smaller files to make the code easier to browse.

OK

mforets commented 2 years ago

For the changes please refer to: https://github.com/JuliaReach/LazySets-JuliaCon21/pull/6


Thanks for the links / DOIs! References [4] and [16] are still missing one, maybe https://jmlr.org/papers/v18/16-107.html and https://arxiv.org/abs/2111.01454 would be fine ?

Fixed, thanks.

OK, maybe add a footnote about this?

We modified the paragraph to include that information.

(In fact the support function of higher dimensional sets is available; the difficult part of the algorithm is where to sample in order to decrease the error bounds. That would probably require some interop with Polyhedra.jl internals.

The new sentences say that the generalization is generally not needed since lazily projecting is available, e.g.

julia> @time P = Projection(rand(Zonotope, dim=10), [5, 10]);
  0.000022 seconds (18 allocations: 2.188 KiB)

julia> matrix(P)
2×10 SparseArrays.SparseMatrixCSC{Float64, Int64} with 2 stored entries:
  ⋅    ⋅    ⋅    ⋅   1.0   ⋅    ⋅    ⋅    ⋅    ⋅ 
  ⋅    ⋅    ⋅    ⋅    ⋅    ⋅    ⋅    ⋅    ⋅   1.0

We didn't include Projection in the article though.)

The blue is almost invisible, can you make it a little thicker or use a different linestyle?

:+1: We updated the plot, using a dashed linestyle, and added a reference to Z and H in the caption of Figure 5.

Much better, maybe just define what a flowpipe is? When I first read I though it was some machine part in a helicopter ^^

Alright, we made a small change in the sentence where it is first used to make it more clear.