sisl / AutomotiveDrivingModels.jl

Driving simulation architecture for Julia
Other
63 stars 29 forks source link

No documentation in docs/Crosswalk.ipynb #6

Closed roryhr closed 7 years ago

roryhr commented 7 years ago

The Crosswalk tutorial has code but lacks the explanatory text of the other two notebooks.

tawheeler commented 7 years ago

Yes - it was written for Maxime as a quick proof of concept. Feel free to fill it out.

MaximeBouton commented 7 years ago

I agree with this and will try to address this by the end of the week

MaximeBouton commented 7 years ago

I added some documentation to the notebook. It consists now in two parts, the first one explaining how to build a T shape intersection, the second explains how to define a crosswalk and pedestrians.

Let me know if some points need more details, I will wait before closing the issue.

roryhr commented 7 years ago

Thanks! A few typos slipped through though.

Where does Roadway() come from? AutomotiveDrivingModels or AutoViz? It makes it hard to tell what's doing what. Explicit is better than implicit so in Python we do

from AutomotiveDrivingModels import Roadway

which in Julia becomes

using AutomotiveDrivingModels: Roadway
MaximeBouton commented 7 years ago

Roadway() is from AutomotiveDrivingModels and is the constructor for the Roadway type.

The only functions and types that come from AutoViz are the one related to display: RenderModel, render, render!, SceneOverlay, FitToContentCamera, add_instruction!, Colorant, RGBA, @colorant_str, render_dashed_line

I am not sure if it should be in this tutorial but I guess as the main documentation is missing we can import them explicitly to make it clearer.

tawheeler commented 7 years ago

@roryhr please be careful to consider the tradeoff between being explicit and implicit. Being explicit is not always advantageous. Writing using AutoViz: RenderModel, render, render!, SceneOverlay, FitToContentCamera, add_instruction!, Colorant, RGBA, @colorant_str, render_dashed_line doesn't help much.

AutoViz only handles the visualization code.

roryhr commented 7 years ago

@tawheeler Your example helps a lot because now I know what you're using from AutoViz. How else would I see that? Seems like a little extra code for a big win to me.

tawheeler commented 7 years ago

Like all software, we want AutomotiveDrivingModels (ADM) to work, be easy to use, and have great documentation. The inherent problem is that documentation has a tendency to become out of date. The same goes for code like the import line above.

Including such detail is a must for a production-quality software. I consider POMDPs.jl to be a great example of such SISL code. BayesNets.jl strives to be that way as well. Many students have spent and continue to spend a lot of time on those packages to develop and maintain them.

ADM has been and I think still is research code. I only recently transferred ownership to SISL proper because other people started using it and I don't want to be singularly responsible for maintaining it. We now have several projects working with it - which is great. That comes with an increased need for stability and documentation. As research code, ADM is still meant to move fast and break things.

I don't want to have to track down all of my imports. Most ADM users import a small number of packages, and almost always use both ADM and AutoViz. If I don't know where a type is defined, I can search for it on my machine or use the @which macro to figure it out. If the code changes I don't need to go back through all of the imports and update them. If Maxime decides to use another scene overlay he needs to update the import statement. Despite everyone knowing or being able to quickly find out that the scene overlays come from AutoViz.

I advocate for the use of using Package: specific_function when there is a conflict. If I have two packages that both export a function, and I only use one of them, I want to make it clear where it comes from. If I need both, I can also call a specific function with Package.function(). But if I use the DataFrames package I do not find it productive to write using DataFrames: Dataframe, writetable, NA as everyone knows where those types and functions come from.

Moving forward we want ADM to be as useful as possible. But there are tradeoffs and we don't want to burden too many people - both new users trying to figure out the code who would love more support and package maintainers that need to fix all of the comments, docs, and examples if the code is changed.

Hopefully I don't come across as aggressive or mean. I am not a professional coder - you are. If you feel strongly about this please use explicit imports, but be prepared to be asked to maintain them as things change.

roryhr commented 7 years ago

I see and agree with your points. There's a compromise to be made, as seen in POMDPs.jl, where the import source is shown in the comments.

using POMDPs
using POMDPModels, POMDPToolbox

# initialize problem (the classic Tiger POMDP)
pomdp = TigerPOMDP() # from POMDPModels

# run a simulation with an ad-hoc policy that always opens the left door