Skip to content
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

[REVIEW]: CFD Python: the 12 steps to Navier-Stokes equations #21

Closed
whedon opened this Issue Jul 2, 2018 · 47 comments

Comments

Projects
None yet
6 participants
@whedon
Copy link
Collaborator

whedon commented Jul 2, 2018

Submitting author: @labarba (Lorena A. Barba)
Repository: https://github.com/barbagroup/CFDPython
Version: v1.0
Editor: @kyleniemeyer
Reviewer: @nicoguaro, @petebachant
Archive: 10.5281/zenodo.1484512

Status

status

Status badge code:

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

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 instructions & questions

@nicoguaro & @petebachant, 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/openjournals/jose-reviews/invitations

The reviewer guidelines are available here: https://jose.theoj.org/about#reviewer_guidelines. Any questions/concerns please let @kyleniemeyer know.

Review checklist for @nicoguaro

Conflict of interest

Code of Conduct

General checks

  • Repository: Is the source for this learning module available at the repository url?
  • License: Does the repository contain a plain-text LICENSE file with the contents of a standard license? (OSI-approved for code, Creative Commons for content)
  • Version: Does the release version given match the repository release (v1.0)?
  • Authorship: Has the submitting author (@labarba) made visible contributions to the module? Does the full list of authors seem appropriate and complete?

Documentation

  • A statement of need: Do the authors clearly state the need for this module and who the target audience is?
  • Installation instructions: Is there a clearly stated list of dependencies?
  • Usage: Does the documentation explain how someone would adopt the module, and include examples of how to use it?
  • Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the module 2) Report issues or problems with the module 3) Seek support

Pedagogy / Instructional design (Work-in-progress: reviewers, please comment!)

  • Learning objectives: Does the module make the learning objectives plainly clear? (We don't require explicitly written learning objectives; only that they be evident from content and design.)
  • Content scope and length: Is the content substantial for learning a given topic? Is the length of the module appropriate?
  • Pedagogy: Does the module seem easy to follow? Does it observe guidance on cognitive load? (working memory limits of 7 +/- 2 chunks of information)
  • Content quality: Is the writing of good quality, concise, engaging? Are the code components well crafted? Does the module seem complete?
  • Instructional design: Is the instructional design deliberate and apparent? For example, exploit worked-example effects; effective multi-media use; low extraneous cognitive load.

JOSE paper

  • Authors: Does the paper.md file include a list of authors with their affiliations?
  • A statement of need: Does the paper clearly state the need for this module and who the target audience is?
  • Description: Does the paper describe the learning materials and sequence?
  • Does it describe how it has been used in the classroom or other settings, and how someone might adotp it?
  • Could someone else teach with this module, given the right expertise?
  • Does the paper tell the "story" of how the authors came to develop it, or what their expertise is?
  • References: Do all archival references that should have a DOI list one (e.g., papers, datasets, software)?

Review checklist for @petebachant

Conflict of interest

Code of Conduct

General checks

  • Repository: Is the source for this learning module available at the repository url?
  • License: Does the repository contain a plain-text LICENSE file with the contents of a standard license? (OSI-approved for code, Creative Commons for content)
  • Version: Does the release version given match the repository release (v1.0)?
  • Authorship: Has the submitting author (@labarba) made visible contributions to the module? Does the full list of authors seem appropriate and complete?

Documentation

  • A statement of need: Do the authors clearly state the need for this module and who the target audience is?
  • Installation instructions: Is there a clearly stated list of dependencies?
  • Usage: Does the documentation explain how someone would adopt the module, and include examples of how to use it?
  • Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the module 2) Report issues or problems with the module 3) Seek support

Pedagogy / Instructional design (Work-in-progress: reviewers, please comment!)

  • Learning objectives: Does the module make the learning objectives plainly clear? (We don't require explicitly written learning objectives; only that they be evident from content and design.)
  • Content scope and length: Is the content substantial for learning a given topic? Is the length of the module appropriate?
  • Pedagogy: Does the module seem easy to follow? Does it observe guidance on cognitive load? (working memory limits of 7 +/- 2 chunks of information)
  • Content quality: Is the writing of good quality, concise, engaging? Are the code components well crafted? Does the module seem complete?
  • Instructional design: Is the instructional design deliberate and apparent? For example, exploit worked-example effects; effective multi-media use; low extraneous cognitive load.

JOSE paper

  • Authors: Does the paper.md file include a list of authors with their affiliations?
  • A statement of need: Does the paper clearly state the need for this module and who the target audience is?
  • Description: Does the paper describe the learning materials and sequence?
  • Does it describe how it has been used in the classroom or other settings, and how someone might adotp it?
  • Could someone else teach with this module, given the right expertise?
  • Does the paper tell the "story" of how the authors came to develop it, or what their expertise is?
  • References: Do all archival references that should have a DOI list one (e.g., papers, datasets, software)?
@whedon

This comment has been minimized.

Copy link
Collaborator Author

whedon commented Jul 2, 2018

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

⭐️ Important ⭐️

If you haven't already, you should seriously consider unsubscribing from GitHub notifications for this (https://github.com/openjournals/jose-reviews) 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/openjournals/jose-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
@whedon

This comment has been minimized.

Copy link
Collaborator Author

whedon commented Jul 2, 2018

Attempting PDF compilation. Reticulating splines etc...
@kyleniemeyer

This comment has been minimized.

Copy link
Contributor

kyleniemeyer commented Jul 2, 2018

@whedon

This comment has been minimized.

Copy link
Collaborator Author

whedon commented Jul 2, 2018

@nicoguaro

This comment has been minimized.

Copy link
Collaborator

nicoguaro commented Jul 3, 2018

@kyleniemeyer, do we expect Statement of need to be explicitly written in the README?

gforsyth added a commit to barbagroup/CFDPython that referenced this issue Jul 3, 2018

gforsyth added a commit to barbagroup/CFDPython that referenced this issue Jul 3, 2018

@kyleniemeyer

This comment has been minimized.

Copy link
Contributor

kyleniemeyer commented Jul 3, 2018

@nicoguaro It doesn't have to be called out explicitly by that name, I think, but the need should be written clearly in the README or docs

@labarba

This comment has been minimized.

Copy link
Member

labarba commented Jul 3, 2018

hmmm... I'm not sure it needs to go in the README? It needs to go in the paper.

@nicoguaro

This comment has been minimized.

Copy link
Collaborator

nicoguaro commented Jul 3, 2018

@labarba, according to my interpretation of the guidelines it should be in the README. And yes, also in the paper … if we follow the checklist.

@labarba

This comment has been minimized.

Copy link
Member

labarba commented Jul 3, 2018

That's pretty funny, because I wrote the guidelines 😬

@labarba

This comment has been minimized.

Copy link
Member

labarba commented Jul 3, 2018

Do you all think it's useful to have such content also in the README? I see the paper as the "advertisement" of the work (à la Claerbout), and the README as instructions to the user.

@labarba

This comment has been minimized.

Copy link
Member

labarba commented Jul 3, 2018

Bear in mind, the journal is new and unique, and we're defining our genre in these very conversations.

@nicoguaro

This comment has been minimized.

Copy link
Collaborator

nicoguaro commented Jul 3, 2018

I think that it is OK if it just appears in the paper. But it appears twice in the checklist as well, and I am following point by point in the review. Maybe it is better to update the checklist.

@labarba

This comment has been minimized.

Copy link
Member

labarba commented Jul 3, 2018

You're right! The checklists were adapted from those for JOSS.

Our only accepted JOSE paper does not include that content in the README:
https://github.com/gboeing/pynamical

@kyleniemeyer Help! What do we do?

@kyleniemeyer

This comment has been minimized.

Copy link
Contributor

kyleniemeyer commented Jul 3, 2018

@labarba why don't we move this conversation elsewhere for now, so we can resolve where we want this content? @nicoguaro it sounds like the paper at least satisfies this item, so let's hold off on the README question for now

@nicoguaro

This comment has been minimized.

Copy link
Collaborator

nicoguaro commented Jul 3, 2018

I reviewed the paper/module and I think that it suits the guidelines of JOSE. I have made some comments on the notebooks on barbagroup/CFDPython#45. There are three boxes that I have not marked in the checklist. Following my comments.

Version

There are different notebooks that have different version numbers. Also, there is not a specific number version given in the repository. This has been mentioned by @petebachant in barbagroup/CFDPython#43.

Installation instructions

There are specific instruction on how to install most dependencies, except for numbapro that seems to be deprecated. Also, I think that something extra needs to be done to get JSAnimation to works as a submodule, maybe something along the lines of (?):

git submodule update --init --recursive

It would not hurt to specify that besides installing dependencies the module needs to be cloned.

Content quality

Some of the "bonus notebooks" need some improvements. See barbagroup/CFDPython#45 for more details.

@petebachant

This comment has been minimized.

Copy link
Collaborator

petebachant commented Jul 10, 2018

Regarding the review checklist, there's a typo in "Does it describe how it has been used in the classroom or other settings, and how someone might adotp it?"

gforsyth added a commit to barbagroup/CFDPython that referenced this issue Jul 11, 2018

gforsyth added a commit to barbagroup/CFDPython that referenced this issue Jul 11, 2018

@kyleniemeyer

This comment has been minimized.

Copy link
Contributor

kyleniemeyer commented Aug 16, 2018

@whedon generate pdf

@kyleniemeyer

This comment has been minimized.

Copy link
Contributor

kyleniemeyer commented Aug 16, 2018

Hi @petebachant @nicoguaro, sorry for disappearing for a while... are you satisfied with the changes made by the authors?

@nicoguaro

This comment has been minimized.

Copy link
Collaborator

nicoguaro commented Aug 16, 2018

@kyleniemeyer, the authors already addressed my comments. Thus, I consider that this article is ready to move forward.

@petebachant

This comment has been minimized.

Copy link
Collaborator

petebachant commented Aug 20, 2018

The paper should be good to go after the final release is tagged (barbagroup/CFDPython#43).

@kyleniemeyer

This comment has been minimized.

Copy link
Contributor

kyleniemeyer commented Aug 21, 2018

thanks @nicoguaro @petebachant. I'm going to take a final look at the paper itself before accepting.

@gforsyth

This comment has been minimized.

Copy link

gforsyth commented Aug 25, 2018

Can I request some clarity re: the releasing procedure? My thought was to tag and release v1.0 to mark the accepted version with a link to the rendered paper. I can do a point-release after the fact, but want to know how hard-and-fast the "must have release" line is.

@labarba

This comment has been minimized.

Copy link
Member

labarba commented Aug 25, 2018

We do the release with all the revisions up to acceptance, at that point we do a Zenodo deposit and get a DOI, we give the DOI here in a comment for the editor to run the command @whedon set <doi> as archive. At that point, publication can proceed. (So the link to the JOSE published paper will need to be added in the README after publication.)

@labarba

This comment has been minimized.

Copy link
Member

labarba commented Aug 26, 2018

One more thing: the version gets captured in the Zenodo metadata if we follow this
https://guides.github.com/activities/citable-code/

@kyleniemeyer

This comment has been minimized.

Copy link
Contributor

kyleniemeyer commented Aug 26, 2018

@gforsyth @labarba sorry for the delay—I have some suggestions and corrections for the paper:

  • In the first paragraph, the sentence "This practical module takes students through 12 steps, one by one, at the end of which they will have programmed a solver for the two-dimensional Navier-Stokes equations, using finite differences." sounds a little off. Perhaps something like "This practical module takes students through 12 steps one by one; by the end, they will have programmed a solver for the two-dimensional Navier-Stokes equations using finite differences."
  • 2nd para: "Guiding the students through these steps (without skipping any), they learn many valuable lessons." -> something like "Students learn many valuable lessons as the modules guides them through these steps (without skipping any)."
  • same paragraph: "numerical diffusion, accuracy and convergence" is missing an Oxford comma (not sure if we have decided whether JOSE follows this [correct] convention, but I do 😄... plus I think the rest of the paper uses this)
  • In the statement of need 1st paragraph, the sentence "This learning module places emphasis on practical experience with programming the solution to fundamental mathematical models that can represent fluid behavior." seems to be missing something; maybe "... programming as the solution..." ?
  • Throughout the paper: please replace the hyphen in "Navier–Stokes" with an en-dash (--). Same for Courant-Friedrichs-Lewy on the second page.
  • Third paragraph in statement of need: "As they become perplexed of this behavior..." -> "As they become perplexed at this behavior...". Also, missing comma in "consistency, stability and convergence".
  • In the fourth paragraph of the statement of need, there are a number of inline URLs. Should any (or all) of these instead be proper citations?
  • In the last paragraph, the second citation "Chen, Kalyuga, and Sweller (2015)" needs to use a different cite command to avoid the doubled parentheses.
  • Reference Ketcheson 2014 is missing some information; here is the full BibTeX entry:
@InProceedings{ ketcheson-proc-scipy-2014,
  author    = { {D}avid {I}. {K}etcheson },
  title     = { {T}eaching numerical methods with {I}{P}ython notebooks and inquiry-based learning },
  booktitle = { {P}roceedings of the 13th {P}ython in {S}cience {C}onference },
  pages     = { 19 - 25 },
  year      = { 2014 },
  editor    = { {S}t\'efan van der {W}alt and {J}ames {B}ergstra }
}
  • Reference Sweller 2006 is missing some information (volume, issue, pages)
@labarba

This comment has been minimized.

Copy link
Member

labarba commented Nov 12, 2018

@whedon generate pdf

@whedon

This comment has been minimized.

Copy link
Collaborator Author

whedon commented Nov 12, 2018

Attempting PDF compilation. Reticulating splines etc...
@whedon

This comment has been minimized.

Copy link
Collaborator Author

whedon commented Nov 12, 2018

@labarba

This comment has been minimized.

Copy link
Member

labarba commented Nov 12, 2018

@nicoguaro, @petebachant — Version will be updated with a release and archive in Zenodo.

@nicoguaro — Apart from the version, you do have a couple of unticked boxes, even though you said above that you're ready to recommend publication.

@kyleniemeyer — I've updated the paper with your editorial suggestions.

@gforsyth — Could you make a release and Zenodo deposit? Thanks!
EDIT -- Release and archive done.

@nicoguaro

This comment has been minimized.

Copy link
Collaborator

nicoguaro commented Nov 12, 2018

@labarba, all the boxes should be marked now.

@labarba

This comment has been minimized.

Copy link
Member

labarba commented Nov 12, 2018

@whedon set 10.5281/zenodo.1484512 as archive

@whedon

This comment has been minimized.

Copy link
Collaborator Author

whedon commented Nov 12, 2018

OK. 10.5281/zenodo.1484512 is the archive.

@labarba

This comment has been minimized.

Copy link
Member

labarba commented Nov 12, 2018

@kyleniemeyer Looks like this is ready to accept and publish!

@kyleniemeyer

This comment has been minimized.

Copy link
Contributor

kyleniemeyer commented Nov 12, 2018

@whedon accept

@whedon

This comment has been minimized.

Copy link
Collaborator Author

whedon commented Nov 12, 2018

Attempting dry run of processing paper acceptance...
@whedon

This comment has been minimized.

Copy link
Collaborator Author

whedon commented Nov 12, 2018

Check final proof 👉 openjournals/jose-papers#5

If the paper PDF and Crossref deposit XML look good in openjournals/jose-papers#5, then you can now move forward with accepting the submission by compiling again with the flag deposit=true e.g.

@whedon accept deposit=true
@kyleniemeyer

This comment has been minimized.

Copy link
Contributor

kyleniemeyer commented Nov 12, 2018

@arfon can you take a look at the DOI in the Crossref deposit XML? It looks like the DOI isn't actually present: https://github.com/openjournals/jose-papers/pull/5/files

@kyleniemeyer

This comment has been minimized.

Copy link
Contributor

kyleniemeyer commented Nov 12, 2018

@arfon nvm, I compared against JOSS articles and it looks the same.

@kyleniemeyer

This comment has been minimized.

Copy link
Contributor

kyleniemeyer commented Nov 12, 2018

@whedon accept deposit=true

@whedon

This comment has been minimized.

Copy link
Collaborator Author

whedon commented Nov 12, 2018

I'm sorry @kyleniemeyer, I'm afraid I can't do that. That's something only editor-in-chiefs are allowed to do.

@labarba

This comment has been minimized.

Copy link
Member

labarba commented Nov 12, 2018

@whedon accept deposit=true

@whedon

This comment has been minimized.

Copy link
Collaborator Author

whedon commented Nov 12, 2018

Doing it live! Attempting automated processing of paper acceptance...
@whedon

This comment has been minimized.

Copy link
Collaborator Author

whedon commented Nov 12, 2018

🚨🚨🚨 THIS IS NOT A DRILL, YOU HAVE JUST ACCEPTED A PAPER INTO JOSE! 🚨🚨🚨

Here's what you must now do:

  1. Check final PDF and Crossref metadata that was deposited 👉 openjournals/jose-papers#6
  2. Wait a couple of minutes to verify that the paper DOI resolves https://doi.org/10.21105/jose.00021
  3. If everything looks good, then close this review issue.
  4. Party like you just published a paper! 🎉🌈🦄💃👻🤘

Any issues? notify your editorial technical team...

@kyleniemeyer

This comment has been minimized.

Copy link
Contributor

kyleniemeyer commented Nov 12, 2018

@labarba @gforsyth congrats, your paper is published!

@nicoguaro, @petebachant: thanks so much with your help reviewing this submission!

@whedon

This comment has been minimized.

Copy link
Collaborator Author

whedon commented Nov 12, 2018

🎉🎉🎉 Congratulations on your paper acceptance! 🎉🎉🎉

If you would like to include a link to your paper from your README use the following code snippets:

Markdown:
[![DOI](https://jose.theoj.org/papers/10.21105/jose.00021/status.svg)](https://doi.org/10.21105/jose.00021)

HTML:
<a style="border-width:0" href="https://doi.org/10.21105/jose.00021">
  <img src="https://jose.theoj.org/papers/10.21105/jose.00021/status.svg" alt="DOI badge" >
</a>

reStructuredText:
.. image:: https://jose.theoj.org/papers/10.21105/jose.00021/status.svg
   :target: https://doi.org/10.21105/jose.00021

This is how it will look in your documentation:

DOI

We need your help!

Journal of Open Source Education is a community-run journal and relies upon volunteer effort. If you'd like to support us please consider doing either one (or both) of the the following:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.