search

openjournals / joss-reviews

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

[REVIEW]: oce: an R package for Oceanographic Analysis #3594

Closed whedon closed 2 years ago

whedon commented 3 years ago

Submitting author: !--author-handle-->@dankelley<!--end-author-handle-- (Dan E. Kelley) Repository: https://github.com/dankelley/oce Branch with paper.md (empty if default branch): Version: 1.6-1 Editor: !--editor-->@kthyng<!--end-editor-- Reviewers: @gonzalobravoargentina, @patrickcgray Archive: 10.5281/zenodo.6325185

:warning: JOSS reduced service mode :warning:

Due to the challenges of the COVID-19 pandemic, JOSS is currently operating in a "reduced service mode". You can read more about what that means in our blog post.

Status

status

Status badge code:

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

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

@gonzalobravoargentina & @patrickcgray, 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/joss-reviews/invitations

The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @kthyng know.

✨ Please start on your review when you are able, and be sure to complete your review in the next six weeks, at the very latest ✨

Review checklist for @gonzalobravoargentina

✨ Important: Please do not use the Convert to issue functionality when working through this checklist, instead, please open any new issues associated with your review in the software repository associated with the submission. ✨

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @patrickcgray

✨ Important: Please do not use the Convert to issue functionality when working through this checklist, instead, please open any new issues associated with your review in the software repository associated with the submission. ✨

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

danielskatz commented 2 years ago

@editorialbot generate pdf

editorialbot commented 2 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

patrickcgray commented 2 years ago

Thanks @danielskatz!

Okay I've basically finished up my run through. I have to admit I'm not super familiar with the oceanography tool landscape in R as a python user primarily but this package seems to cleanly solve many needs in oceanographic analysis, has a robust open-source community surrounding it, and the documentation seems to be up to date and comprehensive. I'm quite impressed by the ability to handle so many common datastreams gracefully. The companion textbook is an excellent resource and will serve many beginning users well.

I installed the develop branch of oce from git in a clean Windows 64 R Studio install running R 4.1.2 and it functioned properly and ran all examples. The only bit not stated in the install instructions is that in order to run the remotes install line from a clean install you need to install Rtools and then remotes. Probably not relevant for most users since they'll have done this, so not sure this is useful information to include. I didn't benchmark it and it might've been my system (which had a lot of analysis products currently in memory) but it took a long time to install, maybe 15 minutes?

I plotted some of my own recent CTD which worked quite well. I did notice that in the plot function when specifying the argument which=13 it threw an error, but which=spice did work in contrast to the documentation. It looks correct here https://github.com/dankelley/oce/blob/adab4b0732d1d33949bd66285678f7c6c1fb2fc6/R/ctd.R#L3300 but maybe elsewhere the cases are mixed up? not sure what's up. Happy to file a bug report if needed. Anyway quite minor and otherwise things worked very well and I explored a couple datasets.

The only other bit that may be worth adding to the paper is that I'm not familiar with other tools, this is one that I am excited to dabble with more even though I'm primarily a python user, but it would be helpful in the paper to add a sentence about other tools attempting to fill a similar gap, or if there aren't other similar packages just a quick line about that and what they do instead.

Overall seems like an excellent package, functions without major issue, and is extremely well documented.

dankelley commented 2 years ago

@patrickcgray - thanks for the detailed and very quickly-done review. I'll look into spots (likely in the main README) where I can mention other tools. Most R users won't even see this website, actually, since the normal case is just to type install.packages("oce"), to get the version on CRAN, the Comprehensive R Archive Network. That's fast, taking perhaps 20 seconds or so, but of CRAN policies dictate against updating more than about twice per year (because all previous versions are permanently available).

If you don't mind, it would help a lot if you could post an issue for that which=13 bug. Just a sentence would be enough. We like to have reports even for things that will involve just a line of code change, because users often search reports to see whether they have encountered a problem that's already been addressed in the "develop" branch.

Again, many thanks for the review.

Dan.

patrickcgray commented 2 years ago

@dankelley that sounds good! If you'll just tag me when you've added to the readme/paper and I'll give it another quick review and can finalize or add more feedback based on that.

dankelley commented 2 years ago

@editorialbot generate pdf

dankelley commented 2 years ago

@editorialbot generate pdf

editorialbot commented 2 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

dankelley commented 2 years ago

@patrickcgray We've updated the manuscript, and also fixed that bug about plot(ctd,which=12) that you so astutely found. More notes are below.

PS. please look at the second article proof. In the first one, we had forgotten to thank you, the other reviewer, and the editor. The bot seems to take just a few minutes to create the article proof so likely there will be a note to that effect, by the time I'm finished typing this.

In response to the helpful and well-informed review by @patrickcgray, we have updated the "develop" branch of oce, as well as our manuscript. An overview of the changes is as follows.

  1. oce now handles the plot(ctd,which=13) case properly. Inspired by this report, and to avoid other plot(...,which) problems, we have expanded the test suite (run by R CMD check, or by clicking an icon in Rstudio) to ensure that plot() handles all the documented textual and numeric which values for the ctd class. (We have also added a check that this function reports a error for attempts to plot something that is neither contained in the object nor computable from the contents.)
  2. The newly added Footnote 1 on page 1 of the manuscript now addresses the status of other tools that are analogous to oce. In a nutshell, we are not aware of any single tool (in any language) that covers the ground that oce covers. Even so, we hope that the link now given in this Footnote will be a useful starting point for Python users.
  3. The newly added Footnote 3 on page 1 of the manuscript now touches on the issue of setting up a system to build oce. We do this by providing a link to an oce wiki page that details the steps required for one particular OS type. Please note that the procedure is not particular to oce; any R package that uses Fortran, C or C++ will require that the user install compilers for those languages. There are online resources to help with this work in general, and we hope that our discussion of one particular case will be a useful addition to these. The good thing is that setting up a system to build one source-based R package will usually cover other packages as well, so this work is not something that has to be repeated unless there are major system changes.
editorialbot commented 2 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

dankelley commented 2 years ago

@patrickcgray -- the bot supposed built up the PDF of the manuscript, but the links it provided do not work for me. They might work for you, though. I don't know how to help with this bot work, unfortunately.

kthyng commented 2 years ago

@dankelley What happens when you click on one of the links in the comment above yours? They are working for me.

dankelley commented 2 years ago

@kthyng Actually, it started working a short time after I posted that. I think there was just a time-delay, of which I had been unaware. Thanks. Dan.

kthyng commented 2 years ago

@dankelley ok great!

@patrickcgray I think we just need you to check on responses to your review and see if you're ready to finalize your review (and check the last box).

patrickcgray commented 2 years ago

Nice work on the quick turnaround, this all looks good to me and addresses all issues raised. @kthyng and @dankelley I've finished my review and checked all remaining boxes. Good work on this project. I'll look forward to seeing it continue to grow.

dankelley commented 2 years ago

@editorialbot generate pdf

I've added another phrase in the acknowledgments. (My usual policy is to add "thank you" notes only after a paper is accepted, but I am not too sure whether I'll have the chance to do that in this case, because this is my first JOSS submission.)

editorialbot commented 2 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

kthyng commented 2 years ago

@dankelley We're ready to wrap up! I read through your paper and didn't see any changes to make. After a couple more steps, I plan to accept this as finished.

  1. please confirm the version of the software that you would like associated with your JOSS paper — feel free to make a new release if you want to.
  2. Archive the current snapshot of your software at a place like Zenodo, then report back here the DOI. Please update the metadata to match the title and author list of your JOSS paper for consistency.
dankelley commented 2 years ago

@kthyng Thanks. In response to your comment:

  1. The version is 1.6-1, that is, the most recent CRAN version as of this date. We do not refer to the version in the text. Did you want me to do that?
  2. I have archived version 1.6-1 on zenodo (the link is https://zenodo.org/record/6325185#.YiCc1N9E3tc) i.e. the DOI is 10.5281/zenodo.6325185. I did not understand your instruction to 'update the metadata'. Is that something that is separate from the paper itself?

As you can see, I do not quite understand what my next steps are. I apologize, if those steps relate to the procedures I used when initially submitting the text. Maybe there's a URL I should be reading, to avoid bothering you?

I apologize for being a bit dense here. Dan.

kthyng commented 2 years ago

@dankelley Sorry I was confusing in my statements! I'll try to clarify on these steps which are always at the end of the review.

  1. At the top of this issue you can see there is a version associated with your software — often this changes as part of the JOSS review as changes are made in the software though in this case it maybe have just progressed since so much time has passed since the review began. I'll update the version from 1.5-0 to 1.6-1 on my end. I now realize I'm not sure where this is presented but it is meant to indicate which version of the software exactly was reviewed in the JOSS paper.
  2. I see your zenodo archive and it looks good. The metadata I mentioned is in fact for the Zenodo archive — we have a small preference for the title and author list in the Zenodo archive matching the JOSS paper for consistency between the two products. In your case, the title and author list almost but don't exactly match. If you prefer them as they are, you can leave them, otherwise you can go into Zenodo and your archive itself and update the title and author list there to match.

Please let me know if you have more questions!

kthyng commented 2 years ago

@editorialbot set 1.6-1 as version

editorialbot commented 2 years ago

Done! version is now 1.6-1

kthyng commented 2 years ago

@editorialbot set 10.5281/zenodo.6325185 as archive

editorialbot commented 2 years ago

Done! archive is now 10.5281/zenodo.6325185

dankelley commented 2 years ago

Thanks for the zenodo hint -- I had been under the impression that edits were not permitted, but it let me correct my error.

Are there other steps for me to do?

kthyng commented 2 years ago

No — at this point I am ready to run the final acceptance command. I'll run the "rough draft" command so you can look through everything one more time.

kthyng commented 2 years ago

@editorialbot recommend-accept

editorialbot commented 2 years ago 
Attempting dry run of processing paper acceptance...
editorialbot commented 2 years ago

:warning: Error preparing acceptance. The generated XML metadata file is invalid.

kthyng commented 2 years ago

@dankelley There have been some changes in the tech at JOSS just recently and I see some evidence of that here! Can you look through the error messages and see if you can determine the problem? If it still won't work, I will try and/or get help from someone who knows more about it.

dankelley commented 2 years ago

It's unhappy about the formats of my issn fields in my .bib file. I'll look at the error log and my .bib file and make corrections. Is there any way I can make the system do further tests, without bothering you? (This might take a few tries.)

kthyng commented 2 years ago

You can compile separately from this issue (see docs here) but also feel free to just do it here — I'll ignore the messages for a bit :)

arfon commented 2 years ago

@dankelley There have been some changes in the tech at JOSS just recently and I see some evidence of that here! Can you look through the error messages and see if you can determine the problem? If it still won't work, I will try and/or get help from someone who knows more about it.

It looks like the Crossref validation is failing on this ISSN: https://github.com/openjournals/joss-papers/pull/3010/commits/b08033650d5451dce4a64f8b2db92fd61b66117e#diff-0cf1feada8e37b446552b475411a1595fb35c56e42863f627b956f39902c338bR118

dankelley commented 2 years ago

I think I see two problems in my .bib file

and will fix them once I do some searching of the sources. (If I cannot find proper values, I'll delete the fields.)

dankelley commented 2 years ago

I have fixed up the bad issn fields. I don't quite know where they came from, but my bibliographic database is fairly old, so I'll blame my typing from years past, as opposed to the entries created by zotero (which is how I set up entries lately).

Anyway, I think the now-uploaded paper might get through these tests a bit more smoothly, given:

$ grep -i issn *bib
    issn = {0098-3004},
    issn = {2296-7745},
    issn = {0705-5900},
dankelley commented 2 years ago

@editorialbot recommend-accept

editorialbot commented 2 years ago

I'm sorry @dankelley, I'm afraid I can't do that. That's something only editors are allowed to do.

dankelley commented 2 years ago

@kthyng actually, the docker method fails for me (maybe something to do with the fact I'm on osx). Maybe you can try @editorialbot recommend-accept again, since it (sensibly) won't let me do that. (I don't think these checks are done when I do @editorialbot generate pdf, based on the fact that I hadn't seen these errors before).

kthyng commented 2 years ago

@editorialbot recommend-accept

editorialbot commented 2 years ago 
Attempting dry run of processing paper acceptance...
kthyng commented 2 years ago

@dankelley yes apparently things that are warnings when generating the PDFs regularly become errors at this stage.

editorialbot commented 2 years ago

:wave: @openjournals/joss-eics, this paper is ready to be accepted and published.

Check final proof :point_right: https://github.com/openjournals/joss-papers/pull/3011

If the paper PDF and the deposit XML files look good in https://github.com/openjournals/joss-papers/pull/3011, then you can now move forward with accepting the submission by compiling again with the command @editorialbot accept

kyleniemeyer commented 2 years ago

@editorialbot accept

editorialbot commented 2 years ago 
Doing it live! Attempting automated processing of paper acceptance...
editorialbot commented 2 years ago

🐦🐦🐦 👉 Tweet for this paper 👈 🐦🐦🐦

editorialbot commented 2 years ago

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

Here's what you must now do:

  1. Check final PDF and Crossref metadata that was deposited :point_right: https://github.com/openjournals/joss-papers/pull/3012
  2. Wait a couple of minutes, then verify that the paper DOI resolves https://doi.org/10.21105/joss.03594
  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 commented 2 years ago

Congratulations @dankelley on your article's publication in JOSS!

Many thanks to @gonzalobravoargentina and @patrickcgray for reviewing this, and @kthyng for editing.

editorialbot commented 2 years ago

:tada::tada::tada: Congratulations on your paper acceptance! :tada::tada::tada:

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

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

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

reStructuredText:
.. image:: https://joss.theoj.org/papers/10.21105/joss.03594/status.svg
   :target: https://doi.org/10.21105/joss.03594

This is how it will look in your documentation:

DOI

We need your help!

The Journal of Open Source Software 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:

dankelley commented 2 years ago

Thanks, @kthyng, @gonzalobravoargentina and @patrickcgray, for your insights, your generosity and your patience. Dan.