JOSS Review

[amoeba]

Hello @elfunesto, I've had a chance to review this work for https://github.com/openjournals/joss-reviews/issues/4955 and would like to request some changes.

I was able to check off everything except the following. Please address them:

• [X] License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
  • [X] Your LICENSE files contain some boilerplate that should be removed (lines 1-16 in each)
• [X] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
  • [X] I don't think this submission currently meets the standard outline in the review criteria. I suggest the authors re-work the three READMEs (top level, one in RCaNModel, and one in RCaNconstructor) so the top level README comprehensively covers the criteria (Statement of need, installation instructions, usage, community guidelines). Links between Markdown docs at the top level and in subfolders would be fine but I'd like to see the top level README be fairly comprehensive.
• [X] Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support
  • [X] I found this in the RCaNmodel README but not in the top level README which is a more common place to see it. Please include this in the top level README re-work requested above.

As I reviewed, I found other places to provide my feedback which aren't captured by the JOSS Review Criteria. You can consider these suggestions:

• [X] License
  • [X] I suggest you add the License field to your `DESCRIPTION file as outlined in https://r-pkgs.org/license.html
• [X] Contribution and authorship
  • [X] While poking around the repo, I noticed your commit log is a bit messy with respect to authors. I suggest coordinating across the group to make sure everyone's git configs are correct. You might also consider rewriting history to clean things up.

    $ git shortlog -sn --all -e
     510  hilaire drouineau <hilaire drouineau@irstea.fr>
      44  elfunesto <43066644+elfunesto@users.noreply.github.com>
      37  Christian Mullon <ch.mullon@gmail.com>
      10  Planque, Benjamin <benjamin.planque@hi.no>
       3  christianmullon <ch.mullon@gmail.com>
       2  Benjamin Planque <benjamin.planque@hi.no>
       2  Drouineau Hilaire <hilaire.drouineau@irstea.fr>
       2  mullon-ch <64130573+mullon-ch@users.noreply.github.com>
       1  Mullon <ch.mullon@gmail.com>
       1  Planque <benjamin.planque@hi.no>

• [X] Automated tests
  • [X] I noticed one of your tests is failing Failure (test-checkPolytopeStatus.R:10): this polytope is unbounded
• [X] Your DESCRIPTION file has default text in the Description field https://github.com/inrae/RCaNmodel/blob/1ff07450912bf7d2314783e80df9d10d58a7b1e7/RCaNmodel/DESCRIPTION#L10
• [X] The repo is quite large. I wasn't able to clone the repository on my home internet connection had to go find a faster one. I noticed you've committed a lot of binary assets to the repo which is likely the cause of the size (>1GB). Unless you have a specific reason to do this, I suggest cleaning the repo up (i.e., rewriting history) and storing assets in more appropriate places such as GitHub Releases or software archives.
• [X] The vignette covered what I think is a reasonable example of using your software but I think it could use quite a bit of attention to make it more readable. Some of this is your choice of API design (return values, offering or not offering print() methods) and some is how the RMarkdown doc is constructed (you could use tables more, where appropriate).

[elfunesto]

Dear @amoeba , Thank you for your feedback. I am trying to revise the package following your recommendations. Small question regarding the license field in the description file. I have already such a field (line 11) in RCaNmodel/DESCRIPTION, isn'it enought?

[elfunesto]

I have used filter repo to remove unused filed, tidy the commit history, and I have used lfs migrate to move binary files to lfs

[elfunesto]

@amoeba Two additional questions:

• the test successfully passes here. It is very weird since it is a very simple test using lpsolve facility. Can you tell please me the output of checkPolytopeStatus(list(A = diag(4)[-4, ], b = rep(1, 3))) ?
• regarding the vignette, we have added an additional Rmarkdown alongside the paper which is I think of better quality than the former. Would it be ok if that Rmardown becomes the new vignette? It is located in paper/barents_SM_RMD.Rmd

[amoeba]

I have already such a field (line 11) in RCaNmodel/DESCRIPTION, isn'it enought?

Yes, I must have missed it. This looks good.

I have used filter repo to remove unused filed, tidy the commit history, and I have used lfs migrate to move binary files to lfs

Thanks. It's a bit better.

the test successfully passes here. It is very weird since it is a very simple test using lpsolve facility. Can you tell please me the output of checkPolytopeStatus(list(A = diag(4)[-4, ], b = rep(1, 3))) ?

I get:

> checkPolytopeStatus(list(A = diag(4)[-4, ], b = rep(1, 3)))
[1] "polytope ok"

regarding the vignette, we have added an additional Rmarkdown alongside the paper which is I think of better quality than the former. Would it be ok if that Rmardown becomes the new vignette? It is located in paper/barents_SM_RMD.Rmd

Maybe this one would be good as a new vignette instead of replacing the current main vignette since it's fairly long.

[elfunesto]

Thank you for your feedback. I will check with the coauthors for the vignette. Regarding the test failure, this is weird. In order to try to reproduce the bug, can I ask on which platform you are working and the version of your package lpsolveAPI? Really sorry to bother you with this...

[amoeba]

I'm on macOS 13.1, arm64 arch. You might consider adding https://github.com/r-lib/actions/blob/v2/examples/check-standard.yaml to your GitHub Actions workflows so check+test run on different platforms and versions.

[elfunesto]

Thanks a lot. I set up the github action

[elfunesto]

Hi @amoeba , Funny discovery: the test was working in most machines, including mac machines from r-hub. One of the co-author as the same machine as you so we were to try... and found what is likely to be a R bug on your architecture! In my code, I have a test with 1e30 which corresponds to infinite value in lpsolve. But looks at the difference between 1.0e30 and 1e30! it is not an epsilon and the problem does not happen with 1e29. Surprising! I implement a workaround in the package.

> any(x0 == 1.0e30)
[1] FALSE

> x0
[1] 1e+00 1e+00 1e+00 1e+30

> any(x0 >= 1.0e30)
[1] TRUE

> 1e30==1.0e30
[1] FALSE

> 1e30-1.0e30
[1] 1.407375e+14

> 1e31-1.0e31
[1] 0

> 1e29*10-1.0e30
[1] 0

[amoeba]

Very interesting. I would have though it was an epsilon issue like you mentioned but the difference is very large. Even code like this (on arm64) is strange to me:

> format(1e22, scientific=FALSE)
[1] "10000000000000000000000"
> format(1e23, scientific=FALSE)
[1] " 99999999999999991611392"

It looks like you've gotten all of my checklist items checked off. I'll make another review pass before this Monday and update my review status.

[elfunesto]

Weird, sounds like an R bug under this architecture? Regarding the review, you may have trouble tu pull some files till wednesday: due to the modifications I made to reduce the size of the repo with the use of lfs file, and the numerous github actions run at each push, I reach the lfs bandwith monthly quota (I did not even know about the existence of such a limit)... This should not happen again in the future but for this month, I either have to pay or to wait next week...

[amoeba]

Thanks for working with me on this @elfunesto, I'm going to close this issue since everything appears to have been done.