Edinburgh-Chemistry-Teaching / Data-driven-chemistry

Creative Commons Attribution 4.0 International
22 stars 2 forks source link

Suggested documentation improvements #37

Closed lucydot closed 1 year ago

lucydot commented 1 year ago

Hello!

I'm working through the review criteria for your JOSS submission: https://github.com/openjournals/jose-reviews/issues/192

Very nicely organised project, and some clear documentation. Some smaller points below

ppxasjsm commented 1 year ago

Thanks for the above points! All excellent! Just a couple of comments:

This is a related course that you may want to reference (developed by researchers in Bath): https://pythoninchemistry.org/ch40208/introduction/about_this_book.html

Yes absolutely right! We even took inspiration from there! I'll add it.

Reference section in README

This was just meant to be the final link to the JOSE publications, hence the TBC I am not sure if it is worth while duplicating information in multiple places.

I think step-by-step installation instructions may be useful given audience is complete novice.

I can add this, the idea was really to encourage novices to use Binder rather than a local install. There was some discussion around this and we weren't sure if we should commit to suggesting conda with a conda environment. Thoughts?

Contributing guidelines are fine, however if you really want to encourage people to contribute

totally agree. I'll see if I can improve this!

I don't think its clear how others might adopt the module for their own teaching - this could be included in the README. I think it would be a case of simply cloning the repo - making it a template repo

sure!

lucydot commented 1 year ago

This was just meant to be the final link to the JOSE publications, hence the TBC I am not sure if it is worth while duplicating information in multiple places.

Ah ok, yes that makes sense to leave empty until link put in

I can add this, the idea was really to encourage novices to use Binder rather than a local install. There was some discussion around this and we weren't sure if we should commit to suggesting conda with a conda environment. Thoughts?

I've had trouble with Binder timing out on students, it is also restrictive in that students can't build on work they have done in previous sessions (as can't save the notebook for use later with the free binder service, as far as I know). Towards the end of the course hopefully students will be keen to code and so at some point will need to move to local install. Perhaps you could link here to step-by-step instructions elsewhere if you don't want to clog up the README with lower priority items.

ppxasjsm commented 1 year ago

Perhaps you could link here to step-by-step instructions elsewhere if you don't want to clog up the README with lower priority items.

I will link to the carpentries resource for this and then give explicit info on how to install this particular environment. (NGLVIEW can be a bit tricky...I've had issues with it.)

I'll mark the References TBC as dealt with if that is ok.

ppxasjsm commented 1 year ago

This commit 4955b7a87e7620548abf5d84b9cdb806e992193a addresses:

I'll add the bath book to the paper as well and make sure carpentries is cited appropiately in paper.md in a future commit.

ppxasjsm commented 1 year ago

Commit 8fe6c80 adds more detailed installation instructions

ppxasjsm commented 1 year ago

Commit 62bea42 adds proper citations to carpentry

ppxasjsm commented 1 year ago

I've expanded on how to contribute with this: d01023a. I don't think I want to add more at this stage and see how it goes.

ppxasjsm commented 1 year ago

I think all points are now addressed here, so closing the issue.