openjournals / joss-reviews

Reviews for the Journal of Open Source Software
Creative Commons Zero v1.0 Universal
722 stars 38 forks source link

[REVIEW]: LilyPad: Real-time two-dimensional fluid/structure interaction simulations written in Processing #394

Closed whedon closed 6 years ago

whedon commented 7 years ago

Submitting author: @weymouth (Gabriel D Weymouth) Repository: https://github.com/weymouth/lily-pad Version: v1.0 Editor: @labarba Reviewer: @alignedleft Archive: Pending

Status

status

Status badge code:

HTML: <a href="http://joss.theoj.org/papers/f7384537852ff1799474627b34231cca"><img src="http://joss.theoj.org/papers/f7384537852ff1799474627b34231cca/status.svg"></a>
Markdown: [![status](http://joss.theoj.org/papers/f7384537852ff1799474627b34231cca/status.svg)](http://joss.theoj.org/papers/f7384537852ff1799474627b34231cca)

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) 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 questions

@alignedleft, please carry out your review in this issue by updating the checklist below (please make sure you're logged in to GitHub). The reviewer guidelines are available here: http://joss.theoj.org/about#reviewer_guidelines. Any questions/concerns please let @labarba know.

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

whedon commented 7 years ago

Hello human, I'm @whedon. I'm here to help you with some common editorial tasks for JOSS. @alignedleft it looks like you're currently assigned as the reviewer for this paper :tada:.

:star: Important :star:

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

To fix this do the following two things:

  1. Set yourself as 'Not watching' https://github.com/openjournals/joss-reviews:

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
scotthmurray commented 7 years ago

General Comments

This project is impressive. Processing is a great tool for building real-time, interactive tools. And the language is designed for beginners (in as much as possible), so I appreciate the choice of Processing as the language used here—to reach a less technical (in terms of coding skills) audience.

I have no expertise in fluid dynamics, however, so I can't comment on the value of the specific modeling techniques used here. It seems reasonable to concur that LilyPad is making these approaches much more accessible to people in this field (students and otherwise).

All of that said, the Processing IDE presents some challenges for a project of this scope. Processing philosophy of coding is that of "sketching in code," and Processing "programs" are called "sketches." Most sketches consist of a single .pde file, as in this included example:

screen shot 2017-09-15 at 6 41 56 pm

However, the IDE does support splitting your code across multiple .pde files. This is typically used to store one class per file, as in this other included example:

screen shot 2017-09-15 at 6 42 00 pm

Note that each .pde file is rendered as a separate tab in the IDE.

The IDE is not intended for advanced use cases, such as having 30+ files per sketch, as LilyPad does:

screen shot 2017-09-15 at 6 38 33 pm

Unfortunately, with so many tabs squished onto the screen, tab titles are obscured, which makes the practical task of navigating the examples challenging. (The workaround is to use the downward arrow at far right.) This is not exactly a critique of LilyPad, but normally the Processing team recommends that advanced users use a third-party IDE like Eclipse. Yet this project is trying to reach a new-to-Processing audience.

Given that, I would recommend repackaging the examples as separate sketches, each in its own folder. Yes, this would probably be more work for the maintainer (and I see how having everything in one monster-sized sketch is more convenient, for that purpose!), but it would be much more accessible to others. If creating separate sketch files sounds like a headache, you could imagine using custom build scripts to, say, extract the relevant pieces from the main LilyPad directory, and repackage each example as its own sketch.

Doing this would avoid the confusion (and errors) that occur when copy-pasting commented-out code into the main LilyPad.pde tab. I found this copy-and-paste instruction unclear, and it took some trial and error to figure out which pieces to copy and where to paste them (which, itself, required overwriting some existing code).

Having separate sketches is "the Processing way" to package code. The Processing team has documentation on how to contribute your own examples for the built-in Examples browser. Simply to make this project more readily accessible by a wider audience, I'd recommend repackaging as described above.

For context, Processing's built-in Examples browser:

screen shot 2017-09-15 at 6 53 56 pm

Review Checklist

I have a few comments about the unchecked items in the checklist above.

Documentation

Example usage: All of the examples are in the code itself, and while the example do contain some inline comments, I would find it extremely beneficial for the primary documentation (on the project wiki) to step through a few of the simplest examples, line by line, and explain what's going on. This may be especially important since the intended audience is new to Processing; they will need clarity on what parts are Processing-specific syntax, and what parts are about the modeling techniques being used.

Functionality documentation: The documentation describes the primary classes used quite generally, but each class’s API is not documented formally. I would expect a list of all fields and methods (including arguments for each, and returned values). Without that documentation, it is a great deal more effort for users to customize the software to their needs.

Automated tests: There is no documentation of this, but, to be fair, Processing is not really set up for automated testing, in the usual sense. When the docs mention letting people "test" new submissions, it could be useful to clarify exactly what's meant here.

Community guidelines: These could also be clarified, especially for this new-to-Processing audience. The docs don't mention how to report problems (perhaps GitHub issues are assumed) or seek support.

Software paper

References: No references were provided.

Conclusion

Wonderful project, and a great use of Processing. All of my comments are purely about packaging and documentation, to help this tool reach the widest possible audience—or to help the existing audience employ it more rapidly and successfully.

labarba commented 7 years ago

@weymouth 👋

The reviewer had some requests for improvements. When are you able to address these?

weymouth commented 7 years ago

I am still trying to figure out if the subfolder idea of @alignedleft can be done. It is simple enough to move items into folders. The trick is to pull this into a new sketch when the users want somehow.

If creating separate sketch files sounds like a headache, you could imagine using custom build scripts to, say, extract the relevant pieces from the main LilyPad directory, and repackage each example as its own sketch.

I can see the appeal of chunking the many files into smaller, more manageable pieces, I'm not sure how to realize this suggestion. Any further ideas from @alignedleft are welcome...

I'm working on transferring information into an example walkthrough and functional documentation.

scotthmurray commented 7 years ago

Sure! I wasn't imagining that you could move items around in folders dynamically. Rather, you could run a script locally, to pre-package the scripts into discrete sketches that could each be run independently — in contrast to the current, single, multi-tabbed sketch, which requires manual copy/pasting.

The "script" I'm proposing wouldn't / couldn't be written in Processing (I don't think). Maybe Python or similar would be more appropriate. The script would just be a convenience (for you). Think of it as doing all that copy-pasting for your audience, in advance.

We use lots of various build scripts for the processing.org website, not to accomplish this exact same task, but to repackage the included Examples in various forms—e.g., this example is packaged with the Processing download itself, but is also repackaged and formatted for display on the website:

https://processing.org/examples/flocking.html

weymouth commented 7 years ago

Got it. Of course, this means the core code (10+ pde files) would need to be copied into each of those example folders. That's why you were worried about maintenance, right? Is there any way to avoid this? Dynamic aliases or something?

Otherwise I guess the development model would be to check if any of those repeated files had been changed before pushing new versions to git. They could also use this to test any changes against each of the example cases.

Do you think that will significantly improve the usage case for research users? It seems like it might get in the way of extending/combining the current examples...

scotthmurray commented 7 years ago

@weymouth I am not really the target audience, since fluid dynamics is not my field. :-) But generally speaking, my thought was simply that it would be easier for new users if they could simply open an example and click "run" to immediately see results. (That's "the Processing way.")

Even if people are really committed to using the software in advance (such as because, say, they are grad students and have been assigned the software as homework or for a project), having the examples nicely packages will help them understand the breadth of what is possible using this tool. Currently, it takes a lot of fussy and careful copy and pasting to explore that. Even for someone familiar with Processing, it may be unclear why the copy/pasting is necessary.

If you don't feel it's worth it to repackage the examples, you might consider making a demo video (like a showreel) that showcases each of the examples briefly in turn.

labarba commented 6 years ago

hi @weymouth — checking in for the status of this review: are you working on improvements? or do you have responses here to the reviewers comments regarding mods you're not doing?

weymouth commented 6 years ago

Sorry for dropping off the radar for a while. I tried (and failed) twice to incorporate the suggested organizational changes to the project. Both attempts significantly hampered the development workflow for anything other than the "main trunk" of the project, and I ended up having to revert back to the current organization and redo the development.

I gave this a solid try since I can completely see the reviewer's point. It would be great to have these examples be plug and play. However, this development style just doesn't seem to be what the processing IDE was set up for. Everything pretty much needs to be in the one sketch or things gets very complicated very quickly.

So, my plan is to clean up the other points raised and make a Youtube tutorial which I'll link to from github.

scotthmurray commented 6 years ago

Sounds like a great compromise. My main goal in my comments was to help increase accessibility of the examples to beginner users. Since Processing is organized around individual sketches, it makes sense to do as much as you can within that limitation. Making a tutorial video and also just using consistent formatting within each tab will help, too. For example, use some sort of unique comment lines to indicate where the user should copy and paste, e.g.:

// *^*^*^*^*^*^*^*^*^*^*^*^*^*^*^*
// *^*^*^*^*^*^*^*^*^*^*^*^*^*^*^*

// …example code here…

// *^*^*^*^*^*^*^*^*^*^*^*^*^*^*^*
// *^*^*^*^*^*^*^*^*^*^*^*^*^*^*^*
labarba commented 6 years ago

@weymouth Do let us know when you'd like the reviewer to have a final look at your improvements, to get the recommendation to accept.

labarba commented 6 years ago

@weymouth -- This submission has been inactive for a while. Are you still working on it?

arfon commented 6 years ago

I've just send @weymouth an email letting him know that if we don't hear back from him in the next couple of weeks we'll assume he no longer wishes to publish this paper.