search

JuliaCon / proceedings-review

7 stars 1 forks source link

[REVIEW]: Modia3D: Modeling and Simulation of 3D-Systems in Julia #43

Closed whedon closed 4 years ago

whedon commented 5 years ago

Submitting author: !--author-handle-->@MartinOtter<!--end-author-handle-- (Martin Otter) Repository: https://github.com/ModiaSim/Modia3D.jl Branch with paper.md (empty if default branch): Version: Editor: !--editor-->@matbesancon<!--end-editor-- Reviewers: @Datseris, @dawbarton Archive:

Status

status

Status badge code:

HTML: <a href="https://submissions.juliacon.org/papers/16d9193656436b4d4f774f73ab34d8c5"><img src="https://submissions.juliacon.org/papers/16d9193656436b4d4f774f73ab34d8c5/status.svg"></a>
Markdown: [![status](https://submissions.juliacon.org/papers/16d9193656436b4d4f774f73ab34d8c5/status.svg)](https://submissions.juliacon.org/papers/16d9193656436b4d4f774f73ab34d8c5)

Reviewers and authors:

Please avoid lengthy details of difficulties in the review thread. Instead, please create a new issue in the target repository and link to those issues (especially acceptance-blockers) by leaving comments in the review thread below. (For completists: if the target issue tracker is also on GitHub, linking the review thread in the issue or vice versa will create corresponding breadcrumb trails in the link target.)

Reviewer instructions & questions

@Datseris, please carry out your review in this issue by updating the checklist below. If you cannot edit the checklist please:

  1. Make sure you're logged in to your GitHub account
  2. Be sure to accept the invite at this URL: https://github.com/JuliaCon/proceedings-reviews/invitations

The reviewer guidelines are available here: https://proceedings.juliacon.org/guide/reviewers. Any questions/concerns please let @matbesancon know.

✨ Please try and complete your review in the next two weeks ✨

Review checklist for @Datseris

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Paper format

Content

Review checklist for @dawbarton

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Paper format

Content

whedon commented 5 years ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @Datseris it looks like you're currently assigned to review this paper :tada:.

:star: Important :star:

If you haven't already, you should seriously consider unsubscribing from GitHub notifications for this (https://github.com/JuliaCon/proceedings-review) repository. As a reviewer, you're probably currently watching this repository which means for GitHub's default behaviour you will receive notifications (emails) for all reviews 😿

To fix this do the following two things:

  1. Set yourself as 'Not watching' https://github.com/JuliaCon/proceedings-review:

watching

  1. You may also like to change your default settings for this watching repositories in your GitHub profile here: https://github.com/settings/notifications

notifications

For a list of things I can do to help you, just type:

@whedon commands

For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:

@whedon generate pdf
whedon commented 5 years ago 
Attempting PDF compilation. Reticulating splines etc...
whedon commented 5 years ago

PDF failed to compile for issue #43 with the following error:

Can't find any papers to compile :-(

matbesancon commented 5 years ago

@dawbarton would you be available to review this paper?

matbesancon commented 5 years ago

@whedon generate pdf from branch JuliaCon2019-paper

whedon commented 5 years ago 
Attempting PDF compilation from custom branch JuliaCon2019-paper. Reticulating splines etc...
whedon commented 5 years ago

PDF failed to compile for issue #43 with the following error:

rm: cannot remove '*.aux': No such file or directory Latexmk: This is Latexmk, John Collins, 17 Jan. 2018, version: 4.55. Rule 'pdflatex': Rules & subrules not known to be previously run: pdflatex Rule 'pdflatex': The following rules & subrules became out-of-date: 'pdflatex'

Run number 1 of rule 'pdflatex'

Running 'pdflatex -recorder "paper.tex"'

Failure to make 'paper.pdf' Collected error summary (may duplicate other messages): pdflatex: Command for 'pdflatex' gave return code 1 Refer to 'paper.log' for details Looks like we failed to compile the PDF

matbesancon commented 5 years ago

@MartinOtter I think you need ref.bib in the paper.yml file

dawbarton commented 5 years ago

Yes, I'd be interested in reviewing this. What is the deadline?

matbesancon commented 5 years ago

The review happens gradually, so you can start and give feedback either in this issue or in the main repo

matbesancon commented 5 years ago

@whedon add @dawbarton as reviewer

whedon commented 5 years ago

OK, @dawbarton is now a reviewer

matbesancon commented 5 years ago

ping @MartinOtter to get the paper compiling

MartinOtter commented 5 years ago

The issue has now been corrected and latexmk should generate a pdf file (seems to be that latexmk did not like the option of a \usepackage{siunitx}).

Note, a pdf-version is also available as https://github.com/ModiaSim/Modia3D.jl/blob/JuliaCon2019-paper/paper.pdf.

matbesancon commented 5 years ago

@whedon generate pdf from branch JuliaCon2019-paper

whedon commented 5 years ago 
Attempting PDF compilation from custom branch JuliaCon2019-paper. Reticulating splines etc...
whedon commented 5 years ago

PDF failed to compile for issue #43 with the following error:

rm: cannot remove '*.aux': No such file or directory Latexmk: This is Latexmk, John Collins, 17 Jan. 2018, version: 4.55. Rule 'pdflatex': Rules & subrules not known to be previously run: pdflatex Rule 'pdflatex': The following rules & subrules became out-of-date: 'pdflatex'

Run number 1 of rule 'pdflatex'

Running 'pdflatex -recorder "paper.tex"'

===========Latexmk: Missing input file: 'siunitx.sty' from line '! LaTeX Error: File siunitx.sty' not found.' Latexmk: Missing input file: 'siunitx.sty' from line '! LaTeX Error: Filesiunitx.sty' not found.' Failure to make 'paper.pdf' Collected error summary (may duplicate other messages): pdflatex: Command for 'pdflatex' gave return code 1 Refer to 'paper.log' for details Looks like we failed to compile the PDF

matbesancon commented 5 years ago

@MartinOtter siunitx.sty seems to be missing, you can use the command i did above to try to compile your paper once it is done

MartinOtter commented 5 years ago

Added siunitx.sty package explicitly

matbesancon commented 5 years ago

@MartinOtter can you compile the paper when you change something?

matbesancon commented 5 years ago

@whedon generate pdf from branch JuliaCon2019-paper

whedon commented 5 years ago 
Attempting PDF compilation from custom branch JuliaCon2019-paper. Reticulating splines etc...
whedon commented 5 years ago

:point_right: Check article proof :page_facing_up: :point_left:

matbesancon commented 5 years ago

@Datseris, @dawbarton I think you can continue the review here

MartinOtter commented 5 years ago

@MartinOtter can you compile the paper when you change something?

Yes:

I clone the branch to a local directory and use the command latexmk -bibtex -pdf paper.tex to generate a paper.pdf file. I run this on Windows 7, after installing Ruby and latexmk.

matbesancon commented 5 years ago

Yes I meant when you push the changes, compile using whedon like I did above to signal that the changes are done

Datseris commented 5 years ago

Hi @matbesancon and @MartinOtter , I would like to state my first round of review.

The installation of the software proceed as planned, and I can confirm that tests pass on my machine (Windows 10, Julia 1.2). I must disclose that I will not be testing nor reviewing any of the plotting claims of the software. This requires you to (1) install a 3rd party software, completely unrelated with the Julia language and (2) share with them your contact details (!?!?).

The paper describing the software is very well written. It has multiple examples and goes in depth in the algorithms used and the mathematics that accompany them. The authors also take care to show how the Julia programming language is taken advantage of in their work. Truthfully, I was very pleased to see such a level of detail and care taken for this paper. Unfortunately, the online documentation is not yet at this level.

Here are my comments:

  1. In the paper and documentation there is a lack of comparison with other software that achieve similar characteristics. The software should be put into context of other existing software, and also attempt to educate the potential user on why they should choose this software, instead of other. I find it hard to believe that Modia3D.jl is the only software that does what it does.
  2. In the start of paragraph 2 the authors "compare" Modia3D.jl with game engines, but this is not a true comparison. They only mention that they use game developing paradigms without going into much depth. Me, as a potential user, should I consider comparing your software with Unreal Engine or with DifferentialEquations.jl ?
  3. I think the authors assume knowledge of what Modia or Modelica is. The reader of the paper may not necessarily have this knowledge (I for example do not). I think the authors should try to describe Modia in a couple of sentences before using it in text.
  4. Both the paper as well as the documentation lack a statement of need. This also ties with my first comment; the authors do not really put the work into context.
  5. There is no discussion about performance but I think the authors should consider providing one. Does this software match the performance of its competitors? Does it exceed it? Furthermore, novel algorithms seem to have been developed and be in use for this software (the three papers associated with the software, e.g. "Collision Handling with Variable-Step Integrators (EOOLT 2017, December)". Do these approaches also offer performance benefits?
  6. The documentation is severely lacking examples as well as instructions. The package features are listed in detail, but instructions of how to leverage them are nowhere to be seen. Specifically, the "getting started page" is simply installation instructions, while all the "examples" of the software are presented like so: 
    # Examples to demonstrate kinematic movement of assemblies
include("$(Modia3D.path)/examples/kinematics/Move_DoublePendulum.jl")
include("$(Modia3D.path)/examples/kinematics/Move_FourBar.jl")
include("$(Modia3D.path)/examples/kinematics/Move_FourBar2.jl")

    This is not enough. At least one example must be expanded in detail and every single step of the example must be explained, also in detail. In fact, at the moment I cannot see how a user would actually use Modia3D.jl, given the absence of guidance. Please be aware that listing an example is not enough: you rely on me being able to deduce what a command does by seeing its output. This is not the proper way to learn, as I might deduce many things wrongly.

  7. Although optional, I think the authors should strongly consider bundling this plotting library they use with their software. The current situation is a no-go for me.

@matbesancon I think you must clarify the following, because a lot of my comments concern documentation and being able to use the software. In many places the authors state that this is "an experimental software" and "not yet ready for production". And in fact the software version is 0.3.0. These statements already justify the lack of documentation.

Please clarify what am I supposed to review here: am I reviewing a paper that explains novel algorithms or am I reviewing a software that I could be potentially using at my work?

matbesancon commented 5 years ago

In many places the authors state that this is "an experimental software" and "not yet ready for production". And in fact the software version is 0.3.0. These statements already justify the lack of documentation.

Having accessible documentation and software that can be used by people other than the main developer(s) is in the criteria for the proceedings.

am I reviewing a paper that explains novel algorithms or am I reviewing a software that I could be potentially using at my work?

We are reviewing both, in the end, the proceedings will store the paper and the DOI corresponding to the version of the software

matbesancon commented 5 years ago

@dawbarton you didn't seem to have a checklist so I added it on the first whedon comment

MartinOtter commented 5 years ago

We will soon release a new version of Modia3D (latest end of next week) that will contain a cleaned-up version of the code used as basis for the paper and also a cleaned-up version of the documentation.

matbesancon commented 5 years ago

It would be nice to wrap things up before here then

matbesancon commented 5 years ago

ping @MartinOtter

dawbarton commented 5 years ago

Apologies, I'm behind on this. I hope to take a proper look tomorrow.

dawbarton commented 5 years ago

Overall, I think this is a well-written paper of an appropriate standard for publication. As someone with a bit of experience in this field, there is nothing I particularly struggled with.

Similar to Datseris' review my main criticism of the paper is the lack of context. What other rigid body dynamics packages should this be compared with? (Open source or closed source - a few reference points would be helpful.) While game engine comparisons are interesting (particularly what can be learnt from the structure of the code), I doubt they are the most appropriate in terms of the scientific objectives. Would software packages such as Mambo (http://danko.mechanical.illinois.edu/Mambo/MamboHome.htm) be an appropriate comparison? (I don't have any experience of it myself.)

Further to the lack of context, there could be a bit more information about the intended use cases of the software. From my own experience I can infer what they are likely to be but making it a bit more explicit would be good.

There are also a number of unqualified assertions in the paper, for example "The advantages of this approach compared to a response calculation with impulses is that (a) simulation results are closer to reality and (b) it is easier to treat complex contact situations correctly."; references supporting these statements would be much appreciated.

The online documentation provided is a little sparse, but well above what I would consider to be the bare minimum. With the information provided, I'd be happy to have a go at creating my own models though I suspect I would end up digging through the source code to find out necessary details.

The remaining item I haven't checked off is the references: the guidelines above appear to indicate that DOIs are required for all relevant publications and as far as I can see there aren't any provided. Other than that, the references look appropriate.

A final comment/question, more from scientific curiousity than anything else, is it possible to use other impact laws other than linear elasticity for the contacts? For example the nonlinear relationships provided by Stronge's Impact Mechanics (Cambridge Press, 2018).

dawbarton commented 5 years ago

I should also note that I have run the examples on Julia 1.3-RC2 (along with the visualisation software) and everything appears to work appropriately.

MartinOtter commented 5 years ago

We will soon release a new version of Modia3D (latest end of next week) that will contain a cleaned-up version of the code used as basis for the paper and also a cleaned-up version of the documentation.

This version of Modia3D (version 0.4.0) was released on Sept. 28.

matbesancon commented 5 years ago

thanks @dawbarton and @Datseris for the reviews. @MartinOtter please make the appropriate changes to the paper, we should be finished soon with the review here

MartinOtter commented 5 years ago

A final comment/question, more from scientific curiousity than anything else, is it possible to use other impact laws other than linear elasticity for the contacts? For example the nonlinear relationships provided by Stronge's Impact Mechanics (Cambridge Press, 2018).

In the previous release of Modia3D, contacts had been defined with linear elasticity. In the release from Sept. 28 and in the paper, contact is defined by a new, nonlinear contact law which is an extension/improvement of existing approaches (including Hertz pressure). The details are documented in the largely improved Modia3D documentation (collision docu)

AndreaNeumayr commented 5 years ago

@matbesancon I added all available DOI's for the reference list (ref.bib) in the paper. The JuliaCon proceedings template is not supporting it. There are no DOI's shown. Do you know how to fix that?

matbesancon commented 5 years ago

yes it's a pity but it's not showing it, some others have reported it. @vchuravy should we modify the bibstyle?

matbesancon commented 5 years ago

@whedon check references

whedon commented 5 years ago 
Attempting to check references...
AndreaNeumayr commented 5 years ago

Thank you, for the reply. I'm sorry. I've not updated the references on github so far.

matbesancon commented 5 years ago

ok, please do it ASAP we will try to wrap up this review

matbesancon commented 5 years ago

bump @AndreaNeumayr @MartinOtter we should finish this quickly now to get in the first issue

MartinOtter commented 5 years ago

bump @AndreaNeumayr @MartinOtter we should finish this quickly now to get in the first issue

I am sorry, we had been very busy. We wil update the paper today or latest tomorrow (DOIs are already updated in the ref.bib file).

AndreaNeumayr commented 5 years ago

@matbesancon We've improved our paper and updated it based on the reviewers comments. We've also improved the documentation of our package and the readme in our latest release of Modia3D v0.4.0. I'm sorry, it took us a bit longer than expected.

matbesancon commented 5 years ago

Ok thanks @dawbarton @Datseris would you like to have another round of review and see if the last criteria & boxes are checked?

matbesancon commented 5 years ago

@whedon generate pdf from branch JuliaCon2019-paper

whedon commented 5 years ago 
Attempting PDF compilation from custom branch JuliaCon2019-paper. Reticulating splines etc...