openjournals / jose-reviews

Reviews for the Journal of Open Source Education (JOSE)
http://jose.theoj.org
Creative Commons Zero v1.0 Universal
33 stars 4 forks source link

[REVIEW]: From Maps to Models - Tutorials for structural geological modeling using GemPy and GemGIS #185

Closed whedon closed 1 year ago

whedon commented 1 year ago

Submitting author: !--author-handle-->@AlexanderJuestel<!--end-author-handle-- (Alexander Jüstel) Repository: https://github.com/cgre-aachen/gemgis_data Branch with paper.md (empty if default branch): Version: 1.0.0 Editor: !--editor-->@acocac<!--end-editor-- Reviewers: @BenjMy, @jwagemann Archive:

Status

status

Status badge code:

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

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

@BenjMy & @jwagemann, 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://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @acocac 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 @BenjMy

Conflict of interest

Code of Conduct

General checks

Documentation

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

JOSE paper

Review checklist for @jwagemann

Conflict of interest

Code of Conduct

General checks

Documentation

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

JOSE paper

whedon commented 1 year ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @BenjMy, @jwagemann it looks like you're currently assigned to review this paper :tada:.

:warning: JOSE reduced service mode :warning:

:star: Important :star:

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

For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:

@whedon generate pdf
whedon commented 1 year ago

Wordcount for paper.md is 2280

whedon commented 1 year ago
Software report (experimental):

github.com/AlDanial/cloc v 1.88  T=2.14 s (31.8 files/s, 52267.7 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Jupyter Notebook                49              0         105485           5289
Markdown                         7            107              0            268
QML                              1              3              0            240
XML                              8              0              0            221
TeX                              1             14              0            168
YAML                             2              5              6             34
-------------------------------------------------------------------------------
SUM:                            68            129         105491           6220
-------------------------------------------------------------------------------

Statistical information for the repository '537c091cba9272b7a32d1dce' was
gathered on 2022/12/01.
No commited files with the specified extensions were found.
whedon 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:

whedon commented 1 year ago
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.5281/zenodo.4569086 is OK
- 10.5194/gmd-12-1-2019 is OK
- 10.21105/joss.01450 is OK
- 10.5281/zenodo.4572994 is OK
- 10.1109/MCSE.2007.55 is OK
- 10.1016/j.pepi.2008.06.013 is OK
- 10.1007/978-94-011-2556-7_11 is OK
- 10.1130/GES02455.1 is OK
- 10.1007/BF02775087 is OK
- 10.1007/978-1-4615-9630-1 is OK
- 10.21105/joss.03709 is OK

MISSING DOIs

- None

INVALID DOIs

- None
acocac commented 1 year ago

👋 @BenjMy & @jwagemann we will conduct the review in this issue.

Please read through the above information and let me know if you have any questions about process.

Thank you 🙏

whedon commented 1 year ago

:wave: @BenjMy, please update us on how your review is going (this is an automated reminder).

whedon commented 1 year ago

:wave: @jwagemann, please update us on how your review is going (this is an automated reminder).

BenjMy commented 1 year ago

Thanks for the reminder, I should be able to send my review by the end of next week.

jwagemann commented 1 year ago

Same here - should be able to submit it next week.

BenjMy commented 1 year ago

@AlexanderJuestel @acocac

I have completed an initial review of the GemGIS tutorial. First I'd like to congratulate the GemGIS dev team for this nice piece of work. I took the measure of all the time and investment made to build the tools and notebooks and make them accessible for everyone.

This package has the potential to make an immediate impact on the scientific communities. There are thoughtful and well-paced tutorial notebooks that provide high-level introductions and more detailed examples of the various methods offered in GemGIS (and Gempy). I am excited both for the package's potential in the long run and possibly to use it in the immediate future.

Below are some of my initial thoughts from the checklist that I will begin creating issues for and linking back to this comment.

Checklist topics

Overall it is a good start but it reads a bit choppy. I found the article very resource exploitation (oil, mining, ...) target-oriented while I think this tool has many other disciplines to reach.

jwagemann commented 1 year ago

@acocac I also concluded my review and added my comments to the issues @BenjMy opened in the target repository.

I'd like to thank the authors for submitting their tutorial to JOSE and I think that the notebooks have a great potential to be a valuable contribution for the GemGIS and GemPy communities. However, at this stage, my evaluation is that the tutorials require a major review as in my opinion pedagogical and educational standards of notebooks are not met.

There is room to improve the structure of the course, but also the quality of the notebooks in terms of including a pedagogical narrative. Once the notebooks have been better structured and improved, the paper submission needs a revision as well. There is also potential to improve structure, clarity and guidance.

My proposals on how to improve the submission are found as a response to the following issues:

AlexanderJuestel commented 1 year ago

Dear @BenjMy, Dear @jwagemann,

thank you very much for your extensive reviews. I will try to resolve all issues during the Christmas break! We certainly have a lot of things to do here!

Cheers Alex

acocac commented 1 year ago

Hello, @AlexanderJuestel 👋 @jwagemann @BenjMy have kindly provided their feedback and suggestions to improve the submission. Please work on your revisions and come back here to let us know when you want reviewers and myself to take a new look. Wishing a happy Christmas break.

AlexanderJuestel commented 1 year ago

Hey @BenjMy, I would need some feedback on the issues you opened in the GemGIS Data Repo to continue with my revisions ;)

Thanks and Cheers Alex

BenjMy commented 1 year ago

Hi @AlexanderJuestel! I'm on vacation break until January 10th. I'll get back to you asap. Cheers,

AlexanderJuestel commented 1 year ago

I copied your @BenjMy list to better track my progress.

Checklist topics

  • General Checks

  • [x] Version: up to now there is no release of the proposed package. --> As agreed in https://github.com/cgre-aachen/gemgis_data/issues/17, there will be no further release of the package as the package does not contain any code but only data. However, we can bump the tag on Github to version 1.0.0 prior to completing the review

  • [x] Contribution and authorship: unclear what role Miguel de la Varga, Nils Chudalla, and Stefan Back have played in the code development (no commits). --> A author statement was added to the JOSE Paper , see https://github.com/cgre-aachen/gemgis_data/commit/32e6fcafb79d17f9801aa5f07730be4f65e3930a

  • Documentation

    • Readers are invited to refer to the GEMGIS core code documentation since there is no specific documentation for the GEMGIS-data repos. I think hosting the datasets and notebooks in a separate repo is perfectly relevant particularly to provide clear guidelines on the data FAIRness.
  • [x] I suggest adding details (add a section in the documentation) covering how FAIR are the dataset provided: --> A section about the FAIR principles was added to the Documentation in https://github.com/cgre-aachen/gemgis/commit/be5d3a2f45475a46a2b96f8d08f7dccaad46dfa5

  • [ ] The LICENCE file covers the GemGIS software rights. But under what Licence are the data distributed? I've seen that the authors cited individually the map source for each notebook but it should also clearly be stated on the README and in the doc. --> The rights to the maps are with the author but he passed away a couple of years ago. The publishing house cannot grant us a license to use the data so I feel it is only appropriate and needed to reference it in every notebook

  • [x] Is there a plan for long-term archiving of the data? into Zenodo for instance? --> A Zenodo Repository will be created upon acceptance of the paper

  • [x] Is there a plan for version-controlled data? data could be fetched (using Pooch for example). --> All files are already stored on a OwnCloud server and accessible via pooch, the notebooks will be adapted to either use the shipped data in the repository or allow the user to download the data for each notebook

    • A statement of need
  • [x] The authors don't identify the target audience in the docs nor in the README. --> The target audience was added to the README, see https://github.com/cgre-aachen/gemgis_data/commit/5f5b1d58b67981ad02b36f7de133c29904c2b3f8

  • [x] The definition of the problem being solved is only part of the doc. Would be also interesting to add it to the README. --> I added a paragraph to the README outlining how structural geological models can be used for a wide range of subsequent applications, see https://github.com/cgre-aachen/gemgis_data/commit/d378a995b3d03fb7b01a8d42da26df90c42ed90d

    • Installation instructions
  • [x] Issues with the badge target link

  • [x] The is no list of dependencies in the README. At least the authors should make clear that Gempy is an optional dependency. --> The main dependencies were listed in the README, GemPy was mentioned as optional but actually needed dependency for the tutorials, see https://github.com/cgre-aachen/gemgis_data/commit/5f5b1d58b67981ad02b36f7de133c29904c2b3f8

  • [x] Error using conda-forge installation (inconsistent env) --> Conda-forge installation was tested on February 7th and it worked without issues for gemgis==1.0.7

  • [ ] Error using pip installation (missing dependencies)

  • [x] In the doc, reorder and put "Installing Gempy (Optional)" at the end of the list. --> The installation instructions were rearranged, see https://github.com/cgre-aachen/gemgis/commit/cd43cd5e8bfa3c1cbeef1c0405b04c38dee1832d

    • Usage
  • [x] I opened an issue with some recommendations.

  • [ ] Error Using Binder when GEMGIS is imported.

    • Community guidelines:
  • [x] Add "Authorship and credit" statement. --> A author statement was added to the JOSE Paper , see https://github.com/cgre-aachen/gemgis_data/commit/32e6fcafb79d17f9801aa5f07730be4f65e3930a

  • [x] I would detail a bit more what can of data can be submitted. Under what terms? --> Please see the community guidelines. What would you add there? https://github.com/cgre-aachen/gemgis_data/blob/main/CONTRIBUTING.md

  • Pedagogy / Instructional design

    • Learning objectives

    Individually each notebook is thoroughly documented to guide the user and it is really much appreciated. Nevertheless, the documentation design with respect to the learning objective can be improved. I recommend the authors articulate the notebooks over sections and subsections. Ideally, I would distinguish within new section tutorials which are Gempy dependent from the others. Also, you could articulate the notebooks using sections following your "Learning Objectives" workflow.

    • Instructional design

    • Video Tutorials are expected and I think it is a great initiative.

  • Jose Paper

Overall it is a good start but it reads a bit choppy. I found the article very resource exploitation (oil, mining, ...) target-oriented while I think this tool has many other disciplines to reach. --> I partially disagree with the statement, that the article is focused on hydrocarbons and mining (professionally, I am not even involved in the HC industry...). Here, the first paragraph of the Statement of need which also includes hydrogeological applications (extraction of fresh water), geothermal applications (extraction of thermal waters), and storage applications (storage of CO2 or hydrogen or nuclear waste):

The subsurface below our feet is utilized in many different ways. We extract fresh water, thermal waters and fossil fuels from it. We exploit the subsurface for its coal, minerals and ores in open-pit mines and underground mines. We store fluids and gases as well as nuclear waste in the subsurface. But to do so, a concept of the structure of the subsurface and its properties is needed to drill into the right formation, to dig or drill into the right direction, to ensure that none of the stored fluid escapes and to ensure that no contaminants leak into surrounding rocks.

  • Statement of needs

  • [x] To me for instance the tools developed can be also associated with environmental risk management (water scarcity, landslides, ...). I am personally interested in this package for Catchment hydrogeology modelling. Authors should broaden this idea in the statement of needs. --> What you have mentioned here are specific applications or how the structures created with GemPy can be further used. I have opened an issue to quickly discuss it there --> https://github.com/cgre-aachen/gemgis_data/issues/23

I like the proof of concept and that the authors already used the software to teach RWTH Aachen. Could it be possible to add (in the "Experience of use in teaching and learning situations" section):

acocac commented 1 year ago

Hi @AlexanderJuestel! I'm on vacation break until January 10th. I'll get back to you asap. Cheers,

@AlexanderJuestel are you still waiting any feedback from @BenjMy?

BenjMy commented 1 year ago

Hi @acocac ! From my point of view, I gave all the feedback needed. But happy to contribute to new ones if necessary!

acocac commented 1 year ago

Hi @acocac ! From my point of view, I gave all the feedback needed. But happy to contribute to new ones if necessary!

@BenjMy thanks for confirming you provided all the feedback needed.

AlexanderJuestel commented 1 year ago

Dear @acocac, dear @BenjMy,

thanks for checking in! I am making progress and the comments are very good! As you can see from the link above (https://github.com/openjournals/jose-reviews/issues/185#issuecomment-1382733498), I am still working on some points here that may take some more or less time. I will try to work on them in a timely manner but my PhD is keeping my quite busy these days ;)

I also have to rework the notebooks still but maybe we can check off some open points once I am done with the none-notebook related issues, also to see what is left :)

Cheers Alex

AlexanderJuestel commented 1 year ago

Dear @acocac,

I would like to ask @jwagemann and @BenjMy to go through their comments and remarks and check what I have already completed. In addition, I am sure that some of the tasks at the top of the issue can be checked off already.

It would be amazing to then have an updated list of what I still have to tackle as things are going all over the place by now

Cheers Alex

acocac commented 1 year ago

Hi @jwagemann and @BenjMy. Following @AlexanderJuestel update, I kindly invite you to revisit the checklists and confirm if all suggestions have been addressed in the resource and/or paper.

AlexanderJuestel commented 1 year ago

Hi @jwagemann and @BenjMy. Following @AlexanderJuestel update, I kindly invite you to revisit the checklists and confirm if all suggestions have been addressed in the resource and/or paper.

There will be open points remaining for sure but looking forward to completing some of the other points already :)

BenjMy commented 1 year ago

Hi @acocac and @AlexanderJuestel ,

Thanks for the update. I will go over the items and tick the completed ones next week. Looking forward to seeing the progress. Cheers,

Ben

BenjMy commented 1 year ago

Hi again @acocac and @AlexanderJuestel ,

I went through the items and validated almost everything except "Pedagogy" and "Instructional Design" where still some efforts are necessary. I guess it is in a good way, I've seen a lot of improvement already for the documentation and readme.

I'll be happy to read again the paper entirely and check the flow of the documentation/notebooks once https://github.com/cgre-aachen/gemgis_data/issues/18 will be tackled.

Well done!

acocac commented 1 year ago

Hi again @acocac and @AlexanderJuestel ,

I went through the items and validated almost everything except "Pedagogy" and "Instructional Design" where still some efforts are necessary. I guess it is in a good way, I've seen a lot of improvement already for the documentation and readme.

I'll be happy to read again the paper entirely and check the flow of the documentation/notebooks once cgre-aachen/gemgis_data#18 will be tackled.

Well done!

Thanks @BenjMy for all your efforts. @AlexanderJuestel please notify both reviewers when you address the suggested issue.

AlexanderJuestel commented 1 year ago

Just as a heads-up, I started reworking the notebook structure of the course material. I especially find the comments of @jwagemann very helpful :)

AlexanderJuestel commented 1 year ago

I am making good progress on reworking the tutorial notebooks. I hope to be done with it in one to two weeks if my work schedule allows for that!

image image

AlexanderJuestel commented 1 year ago

@acocac, @BenjMy, @jwagemann I have just completed the reworking of the notebooks necessary for the tutorial series. I will revise the manuscript a little but everything is ready for you to review it again I would say. I hope I could meet your expectations but are happy to add some more content if needed

Cheers Alex

acocac commented 1 year ago

@acocac, @BenjMy, @jwagemann I have just completed the reworking of the notebooks necessary for the tutorial series. I will revise the manuscript a little but everything is ready for you to review it again I would say. I hope I could meet your expectations but are happy to add some more content if needed

Cheers Alex

Thanks for the update @AlexanderJuestel. I kindly ask @BenjMy @jwagemann to revisit their checklists.

BenjMy commented 1 year ago

@acocac, @BenjMy, @jwagemann I have just completed the reworking of the notebooks necessary for the tutorial series. I will revise the manuscript a little but everything is ready for you to review it again I would say. I hope I could meet your expectations but are happy to add some more content if needed Cheers Alex

Thanks for the update @AlexanderJuestel. I kindly ask @BenjMy @jwagemann to revisit their checklists.

Hello @acocac and @AlexanderJuestel , Thanks for the update. I will review the revision next week.

acocac commented 1 year ago

@jwagemann I'm wondered if you have any updates of your revision. It would be great to then have an updated list accordingly. Thank you!

acocac commented 1 year ago

@acocac, @BenjMy, @jwagemann I have just completed the reworking of the notebooks necessary for the tutorial series. I will revise the manuscript a little but everything is ready for you to review it again I would say. I hope I could meet your expectations but are happy to add some more content if needed Cheers Alex

Thanks for the update @AlexanderJuestel. I kindly ask @BenjMy @jwagemann to revisit their checklists.

Hello @acocac and @AlexanderJuestel , Thanks for the update. I will review the revision next week.

@BenjMy I'm wondered if you have any updates of the revision. We appreciate your hard work!

BenjMy commented 1 year ago

Dear @AlexanderJuestel and Gemgis team,

Congrats again on your hard work and dedication in creating this valuable package. I am certain that it will be enlightening for many users. However, I do have a few minor issues and comments that I would like to bring to your attention.

Let me know if you want me to create issues on the repo directly. Best,

Benjamin

acocac commented 1 year ago

According to a recent e-mail communitcation with @jwagemann, she will have a look at her checklisr after April 18. Thanks for your hard work too!

AlexanderJuestel commented 1 year ago

@BenjMy thanks for your comments! No need to create issues I think :)

I added a little paragraph to the manuscript as our team is expanding the connection to VR applications --> see https://github.com/cgre-aachen/gemgis_data/commit/9e6d97593664d12ef25a1e6d809d12524ecfbc18

I will fix the documentation, it is actually broken currently due to reasons I still have to figure out....

Regarding the organization of the Notebooks, I added a table of contents in the introduction notebook and added folders for each section mentioned in the introduction notebook. Within several of the notebooks, I tried to include some post-processing tasks rather than having a dedicated post-processing section.

image image

AlexanderJuestel commented 1 year ago

I have opened an issue with respect to the failing compilation of the documentation here: https://github.com/executablebooks/sphinx-book-theme/issues/719

Waiting on a fix now :)

acocac commented 1 year ago

@jwagemann I'd appreciate to confirm if you have any estimated date to complete the checklist as discussed.

AlexanderJuestel commented 1 year ago

I have opened an issue with respect to the failing compilation of the documentation here: executablebooks/sphinx-book-theme#719

Waiting on a fix now :)

The docs are working again

acocac commented 1 year ago

I have opened an issue with respect to the failing compilation of the documentation here: executablebooks/sphinx-book-theme#719 Waiting on a fix now :)

The docs are working again

@BenjMy may I ask you to revisit your review checklist and comments according to the latest update from @AlexanderJuestel?

acocac commented 1 year ago

@whedon remind @BenjMy @jwagemann in two days

whedon commented 1 year ago

Reminder set for @BenjMy @jwagemann in two days

BenjMy commented 1 year ago

Hello @acocac and @AlexanderJuestel , Thanks for the update. I will review the revision tomorrow.

BenjMy commented 1 year ago

Hello @acocac and @AlexanderJuestel ,

Thanks to the GemGIS team for their wonderful work. As you can see I ticked all my checklist and all my issues were tackled. I'm already a happy user of GemGIS and hope for the best for its future dev. I have no further comment and I'm looking forward to seeing soon the JOSE paper online.

acocac commented 1 year ago

Hello @acocac and @AlexanderJuestel ,

Thanks to the GemGIS team for their wonderful work. As you can see I ticked all my checklist and all my issues were tackled. I'm already a happy user of GemGIS and hope for the best for its future dev. I have no further comment and I'm looking forward to seeing soon the JOSE paper online.

Amazing. Thanks for all your efforts on this. We hope to complete the review process according to the pending revision by the another reviewer.

acocac commented 1 year ago

@whedon remind @jwagemann in one day

whedon commented 1 year ago

Reminder set for @jwagemann in one day

whedon commented 1 year ago

:wave: @jwagemann, please update us on how your review is going (this is an automated reminder).

jwagemann commented 1 year ago

Hi. I am just about to start the review. Has the paper been updated? I only found the paper generated on 1 December 2022?