Closed editorialbot closed 1 year ago
elbeejay
commented
1 year ago Fantastic, thanks @FrancescoPerrone for the clear review summary. As it looks like you are satisfied with the revisions made, I'd just like to ask that you return to your reviewer checklist above and check off the outstanding items.
Thanks again for your efforts reviewing this submission to JOSS!
FrancescoPerrone
commented
1 year ago Mmm… I thought I did that already? Apologies if not, I’ll check it tonight unfortunately I can’t do it quicker than this.
I’ll keep you updated! F
Sent from Outlook for iOShttps://aka.ms/o0ukef
From: J. Hariharan @.> Sent: Monday, August 7, 2023 12:39:56 PM To: openjournals/joss-reviews @.> Cc: Francesco Perrone @.>; Mention @.> Subject: Re: [openjournals/joss-reviews] [REVIEW]: A Python Tool for Predicting and Assessing Unconventional Rare-Earth and Critical Mineral Resources (Issue #5500)
Fantastic, thanks @FrancescoPerronehttps://github.com/FrancescoPerrone for the clear review summary. As it looks like you are satisfied with the revisions made, I'd just like to ask that you return to your reviewer checklist above and check off the outstanding items.
Thanks again for your efforts reviewing this submission to JOSS!
— Reply to this email directly, view it on GitHubhttps://github.com/openjournals/joss-reviews/issues/5500#issuecomment-1667697259, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AALXCV2ZGJVWNT2MGPKVJ5TXUDHYZANCNFSM6AAAAAAYT6NQU4. You are receiving this because you were mentioned.Message ID: @.***>
jeinsle
commented
1 year ago All – here is my first pass at the review. I am breaking it down by the sections of the checklist
General Checks
Contribution – poking at the github, it looks like most of the work has been done by @justaPCWingo. That said I suspect that this is more to do with how the code was developed and released. - overall, it is hard for me to assess how much work was done by any one team member. From my knowledge of the group the contributors on the Git and the authors make sense.
Functionality
Installation – as per my comment on the installation instructions (below) I have not been able to fully install. I know my MSc student has gotten this to run, so I am suspecting a machine problem.
Functionality – personally still need to test, my student is getting results.
Performance – N/A??
Documentation:
Statement of need – the statement of need in the JOSS manuscript clearly lays out who the audience is. It would benefit the ReadTheDoc introduction to add these sentences to the project description. I would also think that this would help to be placed in the Read me on the Github as it would just tell someone landing on the Github without either manuscript or the ReadTheDocs what the repository is about. It may seem redundant, but this reflects the fact the users will be coming to code at about four different directions and just trying to make a consistent experience.
Installation instructions – on the github I would push to make these slightly more explicit / expanded. I have been trying to install on my system (Ubuntu 20.04.6) and have been unsuccessful. In particular, I am hung up on installing GDAL. If this is meant to only be run on windows machines, please add that to the specification in the installation directions. My recommendation would be for a more step by step set along lines where you tell the user to:
Create an environment
Specify if to use Conda or pip for each package and what order they should be installed in.
My attempt has been using conda, but if I should be taking a different approach this needs to be explicit.
Example usage – is provided both in the original Creason et al manuscript as well as the ReadTheDocs file where the example data is worked through.
Functionality documentation – an autogenerated API is provided. This is proably fine for initial deployment, but contributions by users will likely change what is here.
Automated tests – yes standard auto testing has been set up
Community guidelines – yes, this is fairly straightforward and what would be expected for a project that has just been launched.
Software paper
Summary – yes the summary is clear and points out that this tool does one very specific job. (which is difficult and would be hard to replicate with out)
Statement of need – this is well written and clearly explains who and why this code is needed.
State of the field – it is not clear in this manuscript if there are other tools out there trying for similar approaches. It would help to know if there are other methods but that might be discussed in Creason et al (I need to re-read that again)
Quality of writing –overall this is clearly written document which explains the purpose of the code.
References - again this section is fine.
jeinsle
commented
1 year ago I have a few more things to test and get the GDAL running but that is a task for tomorrow
justaPCWingo
commented
1 year ago Hi @FrancescoPerrone , Thanks for the recommendation and additional comments. I will say that we have some basic CI implemented as part of the github repository, but I can easily see it being fleshed out more as this project continues to evolve. The suggestions of video tutorials, example use cases, and community engagement are solid suggestions, and will look for opportunities to implement these as time and budget permits!
justaPCWingo
commented
1 year ago Hi @jeinsle , I like the suggestion of adding the paper's Statement of need to the readme and user documentation; would this raise concerns of self-plagiarism? Perhaps I'm overthinking it. If not, it would be an easy thing to do.
To answer some of the installation questions, this software was mostly developed under Windows, but there are no platform-specific logic or packages in any of this source code. The CI action used to run pytest is running the Python code under Ubuntu 20.x or 22.x (don't remember which exactly). The environment statement was intentionally left vague since how package installation is handled is largely up to personal preference. The packages required should work whether installed under a conda, Python venv, or raw environment. Package order also shouldn't matter.
There are a pair of files in the root of the repository that may assist with virtual environment building, but your mileage may vary.
conda env create -n URC --file environment.yml.python -mpip install -r requirements.txt.GDAL seems to be an incredible pain to install regardless of the platform, but its the best package to serve the needs of this project. Its hard to give specific instructions since GDAL is so dependent on dynamic libraries (.dll/.so/.dylib) that the issues that can crop up will likely be specific to each installation.
One thing I will do is walk through the setup on my personal Ubuntu machine and let you know what sort of bumps I run into. If we're lucky I'll hit the same pain point and be able to help you out.
RE: State of the field, I did revise the language a bit in response to @jameshgrn by essentially just adding this sentence:
No other Python packages are known to contain the combination of geospatial information systems (GIS) and fuzzy logic support required to directly implement the method defined in Creason et al.
Would this be sufficient to address your concerns?
justaPCWingo
commented
1 year ago Hi @jeinsle ,
Here are some Ubuntu-specific instructions that should hopefully get you around GDAL issues.
For GDAL to work, we need to ensure that the library and the python bindings match. We can do that with the following steps (assume all commands are run from the root directory of the repository in a bash prompt):
apt:sudo apt update
sudo apt install libgdal-dev gdal-datapython3 -mvenv urc_env
source urc_env/bin/activatepip install gdal==$(gdal-config --version)pip install -r requirements.txtFrom here, the command python ./urc_assessment_method.py should launch the program.
jeinsle
commented
1 year ago Hi @jeinsle , I like the suggestion of adding the paper's Statement of need to the readme and user documentation; would this raise concerns of self-plagiarism? Perhaps I'm overthinking it. If not, it would be an easy thing to do.
To answer some of the installation questions, this software was mostly developed under Windows, but there are no platform-specific logic or packages in any of this source code. The CI action used to run pytest is running the Python code under Ubuntu 20.x or 22.x (don't remember which exactly). The environment statement was intentionally left vague since how package installation is handled is largely up to personal preference. The packages required should work whether installed under a conda, Python venv, or raw environment. Package order also shouldn't matter.
There are a pair of files in the root of the repository that may assist with virtual environment building, but your mileage may vary.
- For conda:
conda env create -n URC --file environment.yml.- For venv:
python -mpip install -r requirements.txt.GDAL seems to be an incredible pain to install regardless of the platform, but its the best package to serve the needs of this project. Its hard to give specific instructions since GDAL is so dependent on dynamic libraries (.dll/.so/.dylib) that the issues that can crop up will likely be specific to each installation.
One thing I will do is walk through the setup on my personal Ubuntu machine and let you know what sort of bumps I run into. If we're lucky I'll hit the same pain point and be able to help you out.
RE: State of the field, I did revise the language a bit in response to @jameshgrn by essentially just adding this sentence:
No other Python packages are known to contain the combination of geospatial information systems (GIS) and fuzzy logic support required to directly implement the method defined in Creason et al.
Would this be sufficient to address your concerns?
a) I would not worry about self-plagerism, these are all tools that you are using to market your code afterall. My personal prefernce and experience is that when I work with a product I often want the same information consitantly repeated. It just lets me know what is on the tin and that I am in the right place.
b) state of filed - yes that statment works for me... I may have been working from an older doc so missed it. c) installation woes I will comment and query in another post as I feel that desrives its own space.
jeinsle
commented
1 year ago To answer some of the installation questions, this software was mostly developed under Windows, but there are no platform-specific logic or packages in any of this source code. The CI action used to run pytest is running the Python code under Ubuntu 20.x or 22.x (don't remember which exactly). The environment statement was intentionally left vague since how package installation is handled is largely up to personal preference. The packages required should work whether installed under a conda, Python venv, or raw environment. Package order also shouldn't matter.
There are a pair of files in the root of the repository that may assist with virtual environment building, but your mileage may vary.
- For conda:
conda env create -n URC --file environment.yml.- For venv:
python -mpip install -r requirements.txt.
Hi @justaPCWingo thanks for replying and constructivly improving all this documentation. As to installation and virtual enviroments, I know that there is a lot of variety in how this can be done, especially in python based packages. However, I think from a basic instruction and installation point of view offering something along the lines of:
In our experiance we have found this to be the most reliable method for installing.....
just provides a 'standard' or base level path for getting things working. That way if you have very compotnet developers they can go their own way they should be able to see thorugh the morass of libaries etc.
I did try installing from the yml yesterday and got errors. I have actually used the commands above to get GDAL installed and trying conda to then build the enviroment from the yml... and getting some kind of http errors. Will keep trying.
This leads to the disucssion of would you recommend building via conda or python3 commands?
justaPCWingo
commented
1 year ago Hi @jeinsle , RE: Self-plagiarism: fair enough. I'll look to incorporating the Statement of need's language into the Readme and userdocs. Thanks for the the suggestion for language for providing installation advice. I'll give it a bit of thought on what kind of language and details to include, but will take a stab at providing some statement to that affect.
In my experience, I typically use conda env on Windows, and Python's venv on Linux. I've had better luck with Windows DLLs under conda, but find it too complicated when working on Linux. Again, your mileage may vary.
elbeejay
commented
1 year ago Looks like a valuable discussion @jeinsle and @justaPCWingo, I'll weigh in on the two points briefly:
Re: Self-plagiarism - don't worry about self-plagiarism for this type of documentation, especially if you think adding that text into the readme and code documentation will improve the user experience.
Re: GDAL and installation - this is a common issue for folks doing geospatial work in Python, @justaPCWingo I think you could adopt some of the language from your comment above about the different installation pathways for GDAL and include it in the readme/documentation to help provide users with some guidance on that installation process as @jeinsle suggests above. Alternatively I'm sure a link to some other content about installing GDAL for Python would serve the same goal.
justaPCWingo
commented
1 year ago @elbeejay, @jeinsle : Thanks for the feedback; I'll be working to incorporate your suggestions into the documentation over the next day or two.
justaPCWingo
commented
1 year ago Hi all, I've updated the readme and user documentation with the suggestions provided. Please let me know if that is satisfactory.
Thanks!
elbeejay
commented
1 year ago @FrancescoPerrone I wanted to just touch base with you and remind you to check off your outstanding reviewer checkboxes as I got the impression from your above comment that you meant to do so.
@jameshgrn are there any other revisions you are looking to see in this submission? If not I'd like to ask that you review your checklist once more and check off the outstanding items.
@jeinsle it appears that @justaPCWingo has addressed some of your comments, so I'd ask that you too review the checklist and please update it on your end.
I am hoping to bring this review process to a close soon as I do think @justaPCWingo has received useful feedback which has been incorporated and taken to revise and improve the software package and associated paper.
FrancescoPerrone
commented
1 year ago I think this should be me done with the checklist. Can you confirm?
jameshgrn
commented
1 year ago Hey all, mea culpa for being AWOL. I went through the updated repo and ran tests, everything looks good and works as expected. I want to congratulate the devs on what is a robust, FAIR repo! cheers folks. I've checked everything off!
elbeejay
commented
1 year ago I think this should be me done with the checklist. Can you confirm?
Yes @FrancescoPerrone it all looks good, thanks.
@jameshgrn - no problem, thank you for reviewing this and providing your feedback!
elbeejay
commented
1 year ago @jeinsle I wanted to check in here and ask how the review is going, from your previous messages it sounded like there was a bit more you'd wanted to test out?
Please provide an update so @justaPCWingo can answer any questions or make any additional revisions if needed. Thanks!
jeinsle
commented
1 year ago @elbeejay I just need to sit down a write up. the improved documention is good. I will give it a quick spin this morning and then I think we can sign it all off on my end
jeinsle
commented
1 year ago @justaPCWingo the good news is that I can get the URC to install and launch. However, when I download the esm3.zip file I can not seem the find the ESRI shape files that the UI is looking for even after unzipping ecm_3/PRB- URC Asessment data/DA & DS databases/DA & DS vector database.zip
just a little confussed
justaPCWingo
commented
1 year ago @jeinsle I'll take a quick look to see if anything has changed, or if I can provide better information. I should be able to respond in earnest within a few hours.
justaPCWingo
commented
1 year ago @jeinsle , the first problem I see is ecm_3 is a typo; it should be esm_3. I just pushed changes to address these typos. which files are you having a hard time locating specifically?
I see the shp files PRB_structural_domains.shp and PRB_lithologic_domains.shp in the esm_3/PRB- URC Asessment data/Domains/. Note that this is in the outer zip file (that is the zip file as downloaded from EDX).
The PRB_URC_DA_DS_Database.gdb file is in the inner zip file (esm_3/PRB- URC Asessment data/DA & DS databases/DA & DS vector database.zip).
I don't know why the decision was made to stick a zip file inside another zip file, as I had no involvement in the packaging of this EDX submission. Is that where the confusion is coming from?
jeinsle
commented
1 year ago @justaPCWingo sorry for delay as I do want to close this out sooner than later.
elbeejay
commented
1 year ago @jeinsle if you are satisfied then you are done as it looks like you've finished your reviewer checklist above
elbeejay
commented
1 year ago @justaPCWingo - I'm glad to read positive final notes from all of the reviewers. At this time I'm going to do a final check of the repository and paper, and then I'll have a few final steps I will ask you to complete to square away and finalize the metadata that'll be associated with the JOSS publication.
elbeejay
commented
1 year ago @editorialbot check repository
editorialbot
commented
1 year ago Software report:
github.com/AlDanial/cloc v 1.88 T=0.41 s (231.7 files/s, 150075.5 lines/s)
-------------------------------------------------------------------------------
Language files blank comment code
-------------------------------------------------------------------------------
Python 61 4734 6518 42593
SVG 4 2 0 4152
Qt 4 0 0 2506
Markdown 13 310 0 786
TeX 2 26 0 281
YAML 5 12 13 97
DOS Batch 1 8 1 26
reStructuredText 5 21 53 12
make 1 4 7 9
-------------------------------------------------------------------------------
SUM: 96 5117 6592 50462
-------------------------------------------------------------------------------
gitinspector failed to run statistical information for the repositoryWordcount for paper.md is 1576
elbeejay
commented
1 year ago @editorialbot check references
editorialbot
commented
1 year ago Checking the BibTeX entries failed with the following error:
Failed to parse BibTeX on value "," (COMMA) [#<BibTeX::Bibliography data=[15]>, "@", #<BibTeX::Entry >, {:title=>["Unlocking the Potential of Unconventional Critical Mineral Resources Story Map"], :author=>["Yesenchak, Rachel and Justman, Devin and Bauer, Sophia and Creason, C Gabriel and Gordon, Andrew and Montross, Scott N and Sabbatino, Michael and Rose, Kelly"], :abstractnote=>["An ArcGIS Story Map that provides context and understanding of unconventional critical mineral resource potential."], :doi=>["10.18141/1891489"], :url=>["https://www.osti.gov/biblio/1891489"], :journal=>[""]}, ",", "number", "="]@editorialbot generate pdf
editorialbot
commented
1 year ago :point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
elbeejay
commented
1 year ago @justaPCWingo I've got to ask you to fix the error in the .bib file referenced above regarding the osti_1891489 reference. You should be able to match the syntax of one of the other multi-author references to accomplish this. I'd also ask that you consider explicitly telling users they need to clone the repository locally in order to install it (as from what I can tell this is not installable via pip or conda, and instructions assume access to repository files).
After making revisions you can prompt the bot the way I did above to check the references section.
Once the references are fixed and that check is executed successfuly, I will need you to do the following:
justaPCWingo
commented
1 year ago @elbeejay strange, I'll take a look at the bib and see what's going on. And yes, the code is not presently hosted on pypi or a conda channel. I'll update the instructions as you suggest.
Would the Energy Data Exchange (EDX) qualify as an institutional repository for the purposes of the checklist above? This is where we typically park our public products.
justaPCWingo
commented
1 year ago @elbeejay : I've (hopefully) addressed the issues with the bib, and added a little bit of information to the readme with a sentence pointing out the repo will need to be downloaded as part of the installation process.
There is an existing entry on EDX for a pyinstaller-bundled version of this tool, but it does not share the same name as the paper included in this entry. if the archive is supposed to be just the tagged version of the tool, source, I can either attach to the existing submission (which has a doi) or create a new submission altogether.
Which would be preferrable?
justaPCWingo
commented
1 year ago @elbeejay one more question: should the tagged release be from the branch with the paper and bibliography, or the master branch which doesn't contain JOSS specific assets?
elbeejay
commented
1 year ago @editorialbot check references
editorialbot
commented
1 year ago Reference check summary (note 'MISSING' DOIs are suggestions that need verification):
OK DOIs
- 10.1038/s41586-020-2649-2 is OK
- 10.5281/zenodo.3509134 is OK
- 10.5281/zenodo.5884351 is OK
- 10.18141/1503876 is OK
- 10.18141/1614852 is OK
- 10.1190/INT-2019-0019.1 is OK
- 10.18141/1891489 is OK
MISSING DOIs
- None
INVALID DOIs
- https://doi.org/10.1016/j.coal.2011.11.001 is INVALID because of 'https://doi.org/' prefix
- https://doi.org/10.1016/j.gsf.2018.12.005 is INVALID because of 'https://doi.org/' prefix
- https://doi.org/10.1007/s11053-023-10163-x is INVALID because of 'https://doi.org/' prefix@justaPCWingo you'll need to fix the invalid DOI issues flagged by the bot above.
Regarding the question about EDX, I think the issue is going to be that the archived code is not accessible without creation of an EDX account. It is also unclear to me whether or not EDX creates a DOI (does it?). @kthyng if you don't mind weighing in here that'd be great, but my thought is that we'd expect the archived versions of the code to follow the same guidelines as the code repositories.
@justaPCWingo we can wait on @kthyng to weigh in, but with respect to your other questions we ask that the institutional repository title and authors match the JOSS paper exactly, so in this case that may require a new entry. We want the tagged release to come from the primary development branch that holds the latest code updates reflecting the version of the tool the reviewers approved of.
Thanks
justaPCWingo
commented
1 year ago @elbeejay Thanks for the info FYI, you can download public resources from EDX, but you can't preview the resources; I verified that this was the case. You can optionally create a DOI as part of the submission process on EDX.
Thanks to your responses, I think I know the best course of action here. Zenodo is the way to go in this case. I need to get permission to upload to Zenodo, but since this is just a snapshot of existing publically released code, I don't foresee an issue, just some CYA.
That's odd that the DOI prefix is causing issues; I haven't run into that with other *.bib collators. I'll fix and let you know when its up.
justaPCWingo
commented
1 year ago @elbeejay I pushed fixes for the DOI errors flagged above. Definitely something I screwed up, which at least made it obvious to fix :)
elbeejay
commented
1 year ago @editorialbot check references
editorialbot
commented
1 year ago Reference check summary (note 'MISSING' DOIs are suggestions that need verification):
OK DOIs
- 10.1038/s41586-020-2649-2 is OK
- 10.5281/zenodo.3509134 is OK
- 10.5281/zenodo.5884351 is OK
- 10.18141/1503876 is OK
- 10.18141/1614852 is OK
- 10.1190/INT-2019-0019.1 is OK
- 10.1016/j.coal.2011.11.001 is OK
- 10.1016/j.gsf.2018.12.005 is OK
- 10.1007/s11053-023-10163-x is OK
- 10.18141/1891489 is OK
MISSING DOIs
- None
INVALID DOIs
- NoneThanks to your responses, I think I know the best course of action here. Zenodo is the way to go in this case. I need to get permission to upload to Zenodo, but since this is just a snapshot of existing publically released code, I don't foresee an issue, just some CYA.
This sounds good, let me know when you've been able to do this @justaPCWingo - thanks for being understanding and flexible.
justaPCWingo
commented
1 year ago Hi @elbeejay ,
the tagged version is v1.0.1. the zenodo DOI is 10.5281/zenodo.8319843
Please let me know if there is anything else you need me to do.
kthyng
commented
1 year ago Sounds like you all went with Zenodo, let me know if you still have questions.
elbeejay
commented
1 year ago @editorialbot set v1.0.1
editorialbot
commented
1 year ago I'm sorry human, I don't understand that. You can see what commands I support by typing:
@editorialbot commands
elbeejay
commented
1 year ago @editorialbot set v1.0.1 as version
editorialbot
commented
1 year ago Done! version is now v1.0.1
Submitting author: !--author-handle-->@justaPCWingo<!--end-author-handle-- (Patrick Wingo) Repository: https://github.com/NETL-RIC/URC-Assessment-Method Branch with paper.md (empty if default branch): joss-submit Version: v1.0.1 Editor: !--editor-->@elbeejay<!--end-editor-- Reviewers: @jeinsle, @jameshgrn, @FrancescoPerrone Archive: 10.5281/zenodo.8319843
Status
Status badge code:
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
@jeinsle & @jameshgrn, your review will be checklist based. Each of you will have a separate checklist that you should update when carrying out your review. First of all you need to run this command in a separate comment to create the checklist:
The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @elbeejay 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 ✨
Checklists
📝 Checklist for @jeinsle
📝 Checklist for @FrancescoPerrone
📝 Checklist for @jameshgrn