-
Notifications
You must be signed in to change notification settings - Fork 35
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
pyrolite Submission #20
Comments
Editor's Template Editor checks:
Editor commentsThe license is a variant on BSD / MIT -- Chat with RO about this...Reviewers: 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! |
Thanks @lwasser, I hope you're faring well! I've asked one potential reviewer, and will tag them here shortly if they're interested. |
Hi @lwasser, just a quick update - @charlesll has indicated he's interesting in reviewing this. |
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 |
Happy to help if you are still looking for a reviewer |
Hi, I can review this package. Let me know what is the protocol. Thanks! |
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. |
Thanks @lwasser , |
Hi, please find my review below: Package ReviewPlease check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide
DocumentationThe 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 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
Final approval (post-review)
Estimated hours spent reviewing: 4 Review CommentsPyrolite is without a doubt a very useful package and I will certainly recommend it to my students and colleague. 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. 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 |
@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. |
Package ReviewPlease check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide
DocumentationThe 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 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
Final approval (post-review)
Estimated hours spent reviewing: 3 h Revier CommentsPyrolite 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 Some areas of improvement include:
|
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. |
@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. |
@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. |
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%. |
👋 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! @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!! |
Hi @lwasser . All good I think. |
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!! |
@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. |
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! |
@lwasser sorry for the delay, yes i'm happy with the new version and the paper.md. |
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! |
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! |
@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 |
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. |
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! |
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.
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). |
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!! |
🎆 this was accepted by JOSS. thank you again @arfon !! i'll go ahead and close this issue. |
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
<#20 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABUBNQN22A4MYWC3NUESOLDRVYYRNANCNFSM4KXRXLJQ>
.
|
@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..). |
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:
NOTE: this is different from the form designed for reviewers to sign up to review. Thank you in advance for doing this and supporting pyOpenSci. |
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! |
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:
JOSS 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
Please indicate which category or categories this package falls under:
Explain how the and why the package falls under these categories (briefly, 1-2 sentences):
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:
Encourages better practices throughout these processes, including the use of compositional statistics (i.e. log-transforms).
Implements some common geochemical models and methods to make these easily accessible and reusable (e.g. lattice strain models, orthogonal polynomial decomposition of Rare Earth Element patterns - 'lambdas').
Contains a small database of rock forming minerals for normative calculations and looking up mineral formulae.
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).
@tag
the editor you contacted:#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
paper.md
matching JOSS's requirements with a high-level description in the package root or ininst/
.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
The text was updated successfully, but these errors were encountered: