-
Notifications
You must be signed in to change notification settings - Fork 6
JOSS Review: Minor suggested documentation improvements #17
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
Comments
I have fixed the empty_project.html file. I agree with you that I'm not sure it is super useful, but I decided to leave it there anyway for when users prefer to start with a nice empty file. It's not like the empty project is "in the way" or anything ;D About the last two points: are you asking for anything beyond the already provided JSdocs? https://bramvandijk88.github.io/cacatoo/jsdocs/index.html |
You might have misread my comment about the empty_project.html template - I think it is helpful! I found it really useful in trying out Cacatoo. 😄. Those changes look good! For the third point, it is entirely possible that everything I was looking for is in the JSdocs and I just failed to find it. Is there a list of random number generator methods in there? I looked for a while and couldn't find it. For the last point, I definitely would have found it helpful to have more detail beyond what is currently there. For instance, I can see in the documentation that the third argument to |
Actually, you're right that the external libraries used (the mersenne-twister RNG, the odex library, dygraphs) are not in the JSdocs. These were downloaded precisely in the format as they were available on other repositories, and some of them are minimised or simply were missing the docstrings to begin with. In most cases, I think that's fine, as nobody needs to know how dygraphs works under the hood (and they can look it up if necessary). However, you are on point about the RNG functions being really useful to explain. I've added a page in the DOCs to explain the functions shipped with Mersenne-Twister. Finally, about the contents of the "gridpoint" objects, it's actually not predefined at all. Gridpoint is pretty much an empty object, which only comes with a few functions for copying properties from template-objects. One of the reasons I decided to use javascript for this tool is to allow users to put "whatever" into their grid points. I have modified the docstrings to clarify that a typical user will use the output of getGridpoint to feed to the third argument of setGridpoint. Indeed, there's a lot of flexibility in Javascript, which makes documenting things like this a bit.... fuzzy. :/ |
This looks perfect! Thanks! |
Lastly, I've got a couple suggested additions to the documentation. These aren't necessary in my mind, but I think they would be helpful and increase adoption of this framework (they certainly would have been helpful to me as I was testing it out 😄):
// Set half (50%) of the Gridmodel's grid points to 1 (alive)
), which makes it harder to figure out how it's working. I'd recommend fixing them.getNeighbor
methods return, or what the third argument tosetGridpoint
should look like.This issue is part of my JOSS Review (openjournals/joss-reviews#3948)
The text was updated successfully, but these errors were encountered: