pyOpenSci / software-submission

Submit your package for review by pyOpenSci here! If you have questions please post them here: https://pyopensci.discourse.group/
93 stars 36 forks source link

pyrolite Submission #20

Closed morganjwilliams closed 4 years ago

morganjwilliams commented 4 years ago

Submitting Author: Morgan Williams (@morganjwilliams)
All current maintainers: Morgan Williams (@morganjwilliams)
Package Name: pyrolite One-Line Description of Package: A set of tools for getting the most from your geochemical data. Repository Link: https://github.com/morganjwilliams/pyrolite Version submitted: 0.2.5 Editor: @lwasser
Reviewer 1: @rbeucher
Reviewer 2: @charlesll
Archive: DOI JOSS DOI: DOI Version accepted: v 0.2.7 Date accepted (month/day/year): 06/04/2020


Description

pyrolite provides tools for munging, transforming and visualising geochemical data from common tabular formats. It enables you to recalculate and rescale whole-rock and mineral compositions, perform compositional statistics and create appropriate visualisations and also includes numerous specific utilities (e.g. a geological timescale).

Scope

pyrolite leverages Pandas to enable import, munging and transformation of geochemical data from standard tabular formats, and matplotlib to facilitate common (and some less common) geochemical visualisations. One of the principal project aims is assisting to improve the reproducibility of geochemical research (especially for data-processing steps which often are overlooked or undocumented).

With regards to education, pyrolite is well suited to being incorporated into university-level geochemistry and petrology classes which wish to teach a little Python. The documentation is continually evolving, and more examples and tutorials will gradually be added. It isn't a principal aim of the project, however.

pyrolite is targeted towards geochemists and geoscientists who use geochemical data (chemistry, mineralogy and relevant properties), especially those using lithogeochemistry. pyrolite has been developed principally to enable more reproducible data import, munging, transformation and visualization for geochemical data. In addition to this, pyrolite:

Extensions beyond the core package are also being developed for specific applications or interfaces (e.g. to alphaMELTS).

There is at least one other Python package which has some minor overlap for visualisations (GeoPyTool, which has a GUI-focused interface), but generally there are few open source Python packages for geochemistry (especially on PyPI). pyrolite provides some broader functionality (for both plotting and handling geochemistry) and is designed to be used from an editor or terminal and encourage geoscientists to further develop transferable Python skills. Where practical, the APIs for the tools on which it is built are exposed (e.g. pandas, matplotlib and sklearn).

#17

Technical checks

For details about the pyOpenSci packaging requirements, see our packaging guide. Confirm each of the following by checking the box. This package:

Publication options

JOSS Checks - [x] The package has an **obvious research application** according to JOSS's definition in their [submission requirements](https://joss.readthedocs.io/en/latest/submitting.html#submission-requirements). Be aware that completing the pyOpenSci review process **does not** guarantee acceptance to JOSS. Be sure to read their submission requirements (linked above) if you are interested in submitting to JOSS. - [x] The package is not a "minor utility" as defined by JOSS's [submission requirements](https://joss.readthedocs.io/en/latest/submitting.html#submission-requirements): "Minor ‘utility’ packages, including ‘thin’ API clients, are not acceptable." pyOpenSci welcomes these packages under "Data Retrieval", but JOSS has slightly different criteria. - [x] The package contains a `paper.md` matching [JOSS's requirements](https://joss.readthedocs.io/en/latest/submitting.html#what-should-my-paper-contain) with a high-level description in the package root or in `inst/`. - [x] The package is deposited in a long-term repository with the DOI: [10.5281/zenodo.2545106](https://doi.org/10.5281/zenodo.2545106) *Note: Do not submit your package separately to JOSS*

Are you OK with Reviewers Submitting Issues and/or pull requests to your Repo Directly?

This option will allow reviewers to open smaller issues that can then be linked to PR's rather than submitting a more dense text based review. It will also allow you to demonstrate addressing the issue via PR links.

Code of conduct

P.S. Have feedback/comments about our review process? Leave a comment here

Editor and Review Templates

Editor and review templates can be found here

lwasser commented 4 years ago

Editor's Template

Editor checks:


Editor comments

The license is a variant on BSD / MIT -- Chat with RO about this...

Reviewers: Due date: Thursday April, 30th

hi @morganjwilliams just a note that i have this review on my radar and will get going with finding an editor and reviewers shortly. Everything has slowed down a lot in the past month with the current events. Thank you for your patience!

morganjwilliams commented 4 years ago

Thanks @lwasser, I hope you're faring well! I've asked one potential reviewer, and will tag them here shortly if they're interested.

morganjwilliams commented 4 years ago

Hi @lwasser, just a quick update - @charlesll has indicated he's interesting in reviewing this.

lwasser commented 4 years ago

oh wonderful! thank ou @morganjwilliams let me put a ping out on twitter for a second reviewer while we wait for @charlesll to respond! thank you

rbeucher commented 4 years ago

Happy to help if you are still looking for a reviewer

charlesll commented 4 years ago

Hi, I can review this package. Let me know what is the protocol. Thanks!

lwasser commented 4 years ago

oh awesome!! thank you @rbeucher and @charlesll !! i'll add you as reviewers. so here is how it goes

The review guidelines can be found here. read them over. I need to update that document to contain more recieve pyopensci reviews. when we started we only had ropensci as a model.

When you have completed your review - use the review template so submit your review.

we typically ask for a 3 week turn around on reviews. So if it is possible to complete your review by Thursday April, 30th that would be perfect.

At pyopensci we offer the option of including issues that you open as a part of your review that are then addressed through the review. @morganjwilliams has opted to allow this so feel free to submit issues (or pr's) as need be to address things.

Also he has opted to be considered for JOSS. So when we get to that step, we will ask you to have a look at the paper and ensure the package meets the JOSS requirements. we can discuss that as we are further along.

Thank you both for being willing to review this submission. Please get in touch at any point if you have questions OR suggestions. We are still refining our process so we have been tweaking things in each review.

rbeucher commented 4 years ago

Thanks @lwasser ,

rbeucher commented 4 years ago

Hi, please find my review below:

Package Review

Please check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide

Documentation

The package includes all the following forms of documentation:

rbeucher: This is clearly written in the readthedocs documentation. Please copy and paste to the README

Readme requirements The package meets the readme requirements below:

The README should include, from top to bottom:

rbeucher: Again, this can just be copied from the documentation.

rbeucher: The package as a large range of application, I am not sure how this can be done. Maybe a couple of examples.

rbeucher: There are probably many packages available on Github that do similar things. Pyrolite as broader applications.

Functionality

For packages co-submitting to JOSS

Note: Be sure to check this carefully, as JOSS's submission requirements and scope differ from pyOpenSci's in terms of what types of packages are accepted.

The package contains a paper.md matching JOSS's requirements with:

Final approval (post-review)

Estimated hours spent reviewing: 4

Review Comments

Pyrolite is without a doubt a very useful package and I will certainly recommend it to my students and colleague. I have been impressed by the quality of the package. There has been a lot of work in making sure that the functionalities are well documented, both in the docstrings and on readthedocs. The examples provided covers an impressive range of very useful applications. I only had a quick look at the code but it definitely follows some very good practices and I will likely borrow some ideas.

The package is likely to be difficult to use for python beginners but I think that the author has striken the right balance between flexibility and usage. The pandas and matplolib libraries have been leveraged in a way that will provide future-proof development of the code. As a developer myself, I really appreciate the effort and the quality of the work delivered.

There is an extensive range of tests. All of them except 1 have passed on my local machine. I have spent some time playing with the Jupyter notebook provided on readthedocs. Some markdown could be added to the notebooks but users can largely figure out what is going on thanks to the docstrings available for each function. There is a lot of very interesting functionalities (Unit scaling, Convert to oxyde, reference models, composition etc.).

A few Jupyter notebooks break because of a pandas deprecation. This has already been flagged in the issue tracker #31. This must be fixed before accepting the package.

I have also found a few problems due to some data files missing in the data folder. The files were not listed in the MANIFEST.in. I believe this has been fixed in the dev branch and will have to be merged in the version submitted to PyOpenSci.

I would encourage the author to highlight the package as an excellent ressource for teaching in the README.

Having the option to run the examples with Binder is great. However, I found the binders very long to start and I give up at some point. I have not looked at the binder set up but it would be good to improve that if possible.

I hope you will find that review useful.

Romain Beucher

morganjwilliams commented 4 years ago

@rbeucher thanks for your effort in this review and highlighting a few issues! I'll put out a new release shortly (it will be 0.2.6, incorporating changes currently listed in the changelog 'Development' section) with the majority of these issues addressed and an updated README, and make a note here when it's available.

I've struggled in getting binder to boot up quickly, which is partly due to the relatively heavy dependencies (numpy, pandas, matplotlib, scipy, sklearn), and partly due to the fact that it's likely updated more often than people access the binder links (hence the docker image is rebuilt on-access). Happy to incorporate suggestions to make this easier to work with if anyone has ideas on this front.

charlesll commented 4 years ago

Package Review

Please check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide

Documentation

The package includes all the following forms of documentation:

Charles Le Losq: this needs to be added. The website is good but people also stop on the README when browsing Github. I would suggest adding things in the README on Github like a short description of what this software does, its goals, installation, roadmap, etc.

Only for the stable version, not present for the development version.

This is present on the website.

I would suggest duplicating the contributing guidelines from the website in the CONTRIBUTING.md file on Github.

Readme requirements The package meets the readme requirements below:

The README should include, from top to bottom:

Functionality

I would suggest integrating the tests through nosetests. See https://python-packaging.readthedocs.io/en/latest/testing.html

For packages co-submitting to JOSS

While the package mostly proposes utility functions, its global use can definitely allow developing geochemical models. Therefore, I think it fits JOSS guidelines.

Note: Be sure to check this carefully, as JOSS's submission requirements and scope differ from pyOpenSci's in terms of what types of packages are accepted.

The package contains a paper.md matching JOSS's requirements with:

Final approval (post-review)

Estimated hours spent reviewing: 3 h

Revier Comments

Pyrolite is a useful package in a community lead by Excel or other commercial software.

It will help the manipulation of geochemical data. It is not limited to this discipline, as actually it can help the manipulation, visualization and interpretation of data where ternary plots, chemical conversion and so on are necessary. The plans to link it to python-MELTS, to develop isotopic calculation and so on are also very good news. I particularly encourage the author to make the effort to link it to python-MELTS, as this can be an important source of users.

Some areas of improvement include:

morganjwilliams commented 4 years ago

Thanks @charlesll! I'll put together a to-do list of things to update and add a note here when 0.2.6 is out, as mentioned above. I've added two issues to address some of your later comments (#41 & #42).

On the MELTS front, there's an incomplete wrapper for the alphaMELTS executable at pyrolite-meltsutil.readthedocs.io, but I'm looking forward to working with the newer Python-MELTS API when it's ready to go.

In general most aspects of the matplotlib API used in pyrolite are exposed, and relevant arguments are passed through to the matplotlib (or in the case of ternary plots, mpltern) functions and classes to make sure it integrates well. Necessarily a few things are lost in translation, but let me know if anything in particular is concerning here.

As a comment on the general submission requirements, I'm curious how much duplication is advised to fill out the README etc, just because it might end up being both quite lengthy and slightly more difficult to maintain/keep up to date.

charlesll commented 4 years ago

@morganjwilliams for the README, just add a short version that will be understandable by people already used to Python / coding. But I think it is necessary as those people usually search on Github, and the README is the first thin that appears.

Regarding Matplotlib, all seems fine for now, my comment was mosty to invite you to keep it that way. For instance, I like mpltern because it is a very thin wrapper around Matplotlib to do ternary plots. Personally I know how to use pyplot and it does the job, and I won't invest time in learning another plotting package because I just don't have the time. Even when coding in Julia I use Pyplot. I guess I am not the only one in this situation. This is why if you keep your plotting stuffs close to the MPL API, I think this is best and that it will help the growth of your user-base.

morganjwilliams commented 4 years ago

@charlesll no worries, can do. I feel the struggle about vis libraries, although I've thought about adding an interactive plotting backend option for pyrolite via e.g. hvplot (but that's one for the future, and I need to look at feasibility more). Expect 0.2.6 to be out at the end of the week.

morganjwilliams commented 4 years ago

I've now released pyrolite v0.2.7, with an updated and expanded README, a few new features and some bugfixes (see changelog).

This release addresses two of the potential upgrades suggested by @charlesll (pyrolite#41 and pyrolite#42), the fix for the missing line in MANIFEST.in mentioned by @rbeucher, and a few updates to get test coverage to about 90%.

lwasser commented 4 years ago

👋 everyone!! checking in on this review. it sounds like @morganjwilliams has implemented a new release. @charlesll @rbeucher are you satisfied with the fixes as implemented? is there anything else that we need to check or do to finish up this review. thank you all! just a note that i'll be out of the office until june 1. so i will be unresponsive next week but plan to check back in on June 1 to see if we can wrap this review up.

@morganjwilliams i see that you are interested in submitting this to JOSS. is that still the case? if so you will need to create a paper.md file following JOSS specifications. i see that here: https://github.com/morganjwilliams/pyrolite/tree/master/paper @charlesll @rbeucher did you have a look at that paper as well? many thanks all. once we have ok's on both the review, new release AND the paper we can pass it on to JOSS. the process with JOSS is quick given they accept our review!!

rbeucher commented 4 years ago

Hi @lwasser .

All good I think. I had a look a the paper. It is ready for JOSS.

lwasser commented 4 years ago

awesome thank you for the speedy reply @rbeucher let's see if @charlesll is online as well. i can then ping JOSS here and we can move this through their process!!

morganjwilliams commented 4 years ago

@lwasser Yes, still interested in submitting this to JOSS. Please forward this on once @charlesll has a chance to respond to confirm that he's also had a look at the paper.md and the issues raised above have been addressed.

lwasser commented 4 years ago

hi there @charlesll just checkin in again. are you happy with the review as is as this point? i'd like to ping joss to get the JOSS review started. we are really close to wrapping this up so please respond if you see this ping!

i will email you as well in case you don't always see github pings!

charlesll commented 4 years ago

@lwasser sorry for the delay, yes i'm happy with the new version and the paper.md.

lwasser commented 4 years ago

no worries @charlesll i am so appreciative of your review and feedback. Ok next steps -- 👋 hi @arfon !! We have another package - pyrolite that @morganjwilliams would like to submit to JOSS. can you please tell him what you need to support a JOSS review ? Many thanks!

lwasser commented 4 years ago

Time to wrap things up and get pyrolite officially pyopensci approved! It's time to add a pyopensci badge to pyrolite and get it added to our website!

🎉 Pyrolite has been approved by pyopensci ! Thank you @morganjwilliams for submitting pyrolite and many thanks to @charlesll and @rbeucher for reviewing this package! 😸

There are a few things left to do to wrap up this submission:

This package is going to move on to JOSS for review. i believe @arfon will ask you to implement the items below but let's let him chime in here first!

All --if you have any feedback for us about the review process please feel free to share it here. we are always looking to improve our process. I know things were very slow this round and that is absolutely because of me. I appreciate your patience. We have also been updating our documentation to improve the process so all feedback is appreciated!

lwasser commented 4 years ago

@morganjwilliams sanity check - is 0.2.7 the final approved version of pyrolite? it looks like that was possibly from 24 days ago but i just want to check! i'll them update this issue

morganjwilliams commented 4 years ago

I'll also add my thanks to @rbeucher and @charlesll for your effort reviewing, and @lwasser for keeping things moving! It's been a trying year for most, and I appreciate the time you've spent on this review (and helping to improve pyrolite in the process!), especially given all that's been happening.

@lwasser yes, that's the version with changes as requested from the reviews. I converted the github tag to a release, and for reference the related Zenodo doi is 10.5281/zenodo.3877690 . v0.2.8 won't be far away though.

I'll add the badge to the readme with a hotfix patch shortly. Is there any particular branch you'd like to receive pull requests on? I notice there's add-packages and guess this is to be used, but thought it worth checking. Also let me know if for some reason you'd like separate PR's for the package and contributor info.

lwasser commented 4 years ago

thank you @morganjwilliams ! feel free to fork the repo and submit a pr to the master branch. I can test it locally prior to merging. One single PR is just fine! Thank you for asking!!

Congratulations on being accepted by pyOpenSci!! All we need now is the next steps from @arfon on the JOSS side of things but for pyOpenSci this review is complete. i'll keep the PR open until it runs its way through joss! . And Arfon if you'd like me to direct folks differently (vs pinging you here directly) please say the word!

arfon commented 4 years ago

This package is going to move on to JOSS for review. i believe @arfon will ask you to implement the items below but let's let him chime in here first!

Yes, this sounds like a good list. Please go ahead and submit this to JOSS and mention that it's a pyOpenSci submission on the form.

Congratulations on being accepted by pyOpenSci!! All we need now is the next steps from @arfon on the JOSS side of things but for pyOpenSci this review is complete. i'll keep the PR open until it runs its way through joss! . And Arfon if you'd like me to direct folks differently (vs pinging you here directly) please say the word!

This is fine - we can also just have pyOpenSci authors submit without this heads-up directly to me. Submissions from rOpenSci usually just mention this on the form and we act accordingly (authors also usually mention this when the JOSS pre-review issue is opened).

lwasser commented 4 years ago

ok perfect. thank you @arfon i'll add this to our documentation for editors that we can give instructions for folks o submit directly to JOSS and mention that the review happened here already and the package was accepted. Many thanks. So @morganjwilliams please go ahead and submit to JOSS. In your submission, please mention that this went through and was approved by the pyopensci review process. you can even link to this issue. If anything comes up along the way please let me know! i'll close this issue for now but can reopen if need be.

we are actively working on improving our documentation so it's helpful to know how this part of the process goes IF there are tips that we can provide to maintainers like yourself to make things easier!!

lwasser commented 4 years ago

🎆 this was accepted by JOSS. thank you again @arfon !! i'll go ahead and close this issue. @rbeucher and @charlesll are you comfortable with my adding you to the pyopensci website as reviewers? it will show your github use profile picture, name the the package you reviewed.

see: https://www.pyopensci.org/contributors/

charlesll commented 4 years ago

Yes, no problem, feel free to do this!

Le mar. 9 juin 2020 à 15:13, Leah Wasser notifications@github.com a écrit :

🎆 this was accepted by JOSS. thank you again @arfon https://github.com/arfon !! i'll go ahead and close this issue. @rbeucher https://github.com/rbeucher and @charlesll https://github.com/charlesll are you comfortable with my adding you to the pyopensci website as reviewers? it will show your github use profile picture, name the the package you reviewed.

see: https://www.pyopensci.org/contributors/

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/pyOpenSci/software-review/issues/20#issuecomment-641284315, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABUBNQN22A4MYWC3NUESOLDRVYYRNANCNFSM4KXRXLJQ .

morganjwilliams commented 4 years ago

@lwasser just a quick note that the pyOpenSci --> JOSS process was very straightforward and speedy. The info on the website and in the submission/review templates made sure we had everything ready for JOSS when we initially submitted to pyOpenSci.

If you frequently have submissions which would also like to submit to JOSS, consider templating a short paragraph to add to the 'notes to editor' section on the JOSS submission form [e.g. "This has been reviewed and accepted by pyOpenSci (link to issue), and is currently archived on Zenodo/Figshare/other (link to archive or DOI)"]. This won't save much time, but will just make sure info that needs to be communicated is automatically included (you might even be able to prefill some of this data with a query attached to the JOSS submission URL, although my quick test of this didn't quite get there..).

lwasser commented 2 years ago

hey 👋 @morganjwilliams @charlesll @rbeucher ! I hope that you are all well. I am reaching out here to all reviewers and maintainers about pyOpenSci now that i am working full time on the project (read more here). We have a survey that we'd like for you to fill out so we can:

🔗 HERE IS THE SURVEY LINK 🔗

  1. invite you to our slack channel to participate in our community (if you wish to join - no worries if that is not how you prefer to communicate / participate).
  2. Collect information from you about how we can improve our review process and also better serve maintainers. The survey should take about 10 minutes to complete depending upon how much you decide to write. This information will help us greatly as we make decisions about how pyOpenSci grows and serves the community. Thank you so much in advance for filling it out.

NOTE: this is different from the form designed for reviewers to sign up to review. If there are other maintainers for this project, please ping them here and ask them to fill out the survey as well. It is important that we ensure packages are supported long term or sunsetted with sufficient communication to users. Thus we will check in with maintainers annually about maintenance.

Thank you in advance for doing this and supporting pyOpenSci.

lwasser commented 2 years ago

hey there @charlesll @rbeucher 👋 Just a friendly reminder to take 5-10 minutes to fill out our survey . We really appreciate it. Thank you in advance for helping us by filling out the survey!! 🙌

✨ Morgan thank you so much for taking the time to fill it out 🙌 I will shoot you an email later this week!

🔗 HERE IS THE SURVEY LINK 🔗