ObsPy: Software Submission for Review #16
Comments
|
Code of conduct was added after last stable release, it's in |
|
awesome! thanks @megies for this submission. we will get back to you soon!! likely after agu :) in the meantime are there people who you could suggest as reviewers? i can also dig into our spreadsheets but if you know of folks, please feel free to suggest here. now that i'm writing this i'm realizing we might consider adding this question to our template as well. Editor's Template Editor checks:
Editor commentsI asked the submitter below if they were interested in JOSS! Reviewers: Listed above with a due date of feb 24 note: i think it might be good to add the editor checks somewhere in the issue ?? it seems redundant to list reviewers twice and a due date at the bottom. |
That'd be somebody working in seismonology, good in Python and not using obspy? Is that even possible? 👆 just kidding 😜 but yeah, most people I could come up with would be biased 😉 |
|
hey there @megies !! i'm just catching up on things with the holidays, and beginning of the semester here... so my apologies for the delay. Our next meeting is this thursday so i'll be sure to get this review going. I'll put a few calls out to try to find some reviewers. it seems to me that someone who uses obspy could be ok?! but if they are closely related to the development already that would not be ok. let's see if we can use a combo of twitter and our forum to find a few reviewers. more in a bit. i'm hoping to play catch up over the next week and this is a priority for me as it's been sitting for a month. thank you for your patience!! |
|
Like I said, it's all good, we're in no hurry about this at all. 👍 |
|
Hi, I saw the call for reviewers on twitter, all for open source science, would like to be helpful. I'm not in seismology and don't use ObsPy, but do help maintain ArviZ and PyMC3 which I hope qualifies me enough. Let me know if I meet the bar. |
|
I could help with a review. Not specifically known in seismology but I am a hydrogeologist and (co-) develop some python packages. Let me know if help is needed :) |
|
hi @canyon289 and @raoulcollenteur if you have experience developing packages, and are on the geosciences end of things, I think that could work well for this review. @megies please let me know what you think about two people reviewing who are not from the seismology community explicitly? i suspect this might be challenging to find someone without a conflict of interest and such. I know both are technically qualified based upon the comments above!! Thank you all for your responses. |
I don't see a problem with that at all. 👍 |
|
awesome @raoulcollenteur and @canyon289 if you are still willing, I am going to assign you as reviewers for obspy!! Please plan to have your feedback to @megies by Monday Feb 24th. You can use our review guide as a basis for your review. https://www.pyopensci.org/dev_guide/peer_review/reviewer_guide.html We actually have some reviews that you can look out now (our guide is dated already!!) which include: @megies did check they are ok with issues being submitted directly to the obspy repo! so if you'd like you can organize that review with issues providing commentary here and then linking to issues that you open in obspy. how you prefer to work is up to you! If you have any questions please get in touch here!! Thank you again for volunteering your time to review this package! It is a very widely used package in this domain looking at the package download stats on conda so we are excited to see it submitted here! Have a wonderful weekend!! |
|
@megies please excse me if i already asked you this? are you interested in a JOSS citation? it's not a significant amount of extra work. you write the "paper" which can be quite short. earthpy has one in it's repo, and then joss automatically accepts the review that happens here so it's fast once the paper is complete. You then get a more formal citation through a journal which is really nice and linked to your orcid ID (and any other contributors that youd like to add). this is an option so just let me know if you have any interest in it! i'm excited to get this review going! |
|
one other note -- @raoulcollenteur and @canyon289 the review template is here in case you didn't see it above! https://www.pyopensci.org/dev_guide/appendices/templates.html |
|
Great. I'll get this done by the 24th. Also interested in whether this is getting submitted to JOSS as there seem to be extra reviewer items for me to go through if that's the case. |
No problem. I'm not sure if it would do much though, since we have three citable papers for obspy but if you have a good idea how to theme it or if you think it's still adding something, let me know. :-) |
You can start on those items not affected by JOSS submission :-) |
|
awesome. thanks all!! sure. i'll actually reach out to @arfon on this. Arfon if you happen to see this - does JOSS provide anything new in terms of citation and visibility if a tool has already been published in a few other journals? |
👋 If you already have a paper (or multiple papers!) that you like people to cite for this software then I don't think it makes much sense to publish a paper with JOSS. |
|
Thank you @arfon !! that was fast. ok great.. i just emailed you about this given i have a similar question for another review underway! |
|
Hi, For the author
For the editors
In depth comments and examples are in this google doc |
|
Sorry for the slightly late response, but here it is! I think the authors did a great job on the package . I have a couple of comments which I think would improve the introduction of the package for new users. Please find those at the end of the review. Package Review
DocumentationThe package includes all the following forms of documentation:
Readme requirements
Functionality
Final approval (post-review)
Estimated hours spent reviewing: Review CommentsI am not a seismologist by training so I will limit to software technical/UI review. ObsPy is a Python package targeted at seismologists and seems to have a large and active user base. I think this is better proof for the need and usability of this package than anything else. I congratulate the authors on what seems to me a well accepted Python package. The Wiki and documentation are extensive. Some of the information feels a little dispersed, maybe these two sources could be merge to one website in the future to improve easy access to all these resources. The gallery gives a nice quick overview of the possibilities, including the necessary code. In addition to all these resources there are multiple articles introducing the software that provide a quick introduction. I also found a couple of useful repositories in the organisation account (e.g., https://github.com/obspy/docs) that include notebooks, these could be promoted from the main repository as well. Major comments
Minor comments
|
|
Thanks for the feedback, I'll try to have a detailed look as soon as I can.. we're pushing for a major release right now, so might not be able to reply immediately. |
|
Here's my reply to @canyon289, thanks for the helpful comments. I also left some remarks in the google docs, mainly about those failing tests in CI. 1We quite often have tests failing which most times is due to the tests being so rigorous. I can make you a log of numpy floating point operation result differences at the 15th digit and their changes across platforms, architectures and numpy versions. Fun aside, all of the fails you linked are image test fails, you can actually check out the images uploaded to imgur from the traceback message. Usually it's ticks moving around a bit. 2Usage examples could be found following the from obspy import read
st = read() # load example seismogram
st.filter(type='highpass', freq=3.0)
st = st.select(component='Z')
st.plot()
|
|
@raoulcollenteur, thanks for your review, here's my comments/replies:
|
|
@lwasser responded to both reviews, let me know if you need more from my side |
|
thank you @megies !! i'll look at this a bit more closely today! It's becoming clear to me that we need to more carefully consider levels of documentation for good, better and best . The issue of API documentation with examples for instance has come up multiple times. I'm curious what your take is on this. We have an discussion on it in discourse started here. @canyon289 and @raoulcollenteur i'm very interested in your thoughts as well if you have any input! All - thank you for supporting / going through review process and for your patience as we develop our policies and guidelines! |
|
also @canyon289 and @raoulcollenteur can you kindly have a look at the comments above and let me know if you are satisfied with @megies responses? i see that you guys did go back and forth in the google doc! i will copy that text over to this issue just so we have a record of it now (in case the doc gets lost in the future!). |
THIS IS THE GOOGLE DOC REVIEW TEMPLATE - from @canyon289 copying it here for records and safe keeping!!Review Template Checklist Legend 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
No conflict of interest! DocumentationThe package includes all the following forms of documentation:
Short description is available on README. Link to a very comprehensive presentation is linked on README to youtube.
But there are a number that are very well documented
Readme requirements
The README should include, from top to bottom:
Badge for review has not been included yet but everything else has
Functionality
Tests do not pass on my local machine with fresh installation from master (commit: 34c70be39b71c6ec224a38045bb2a89a75f56a76) When pyimgur is installed tests then fail because they’re hitting a web api that is sending back 400. http://tests.obspy.org/109013/ This test failure service you’ve implemented though is really cool.
CI integration exists for appveyor and Travis but Travis seems to be failing at the moment
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:Review CommentsFix: Develop installationDeveloper installation isn’t straightforward. For editable mode. It seems that numpy is needed prior to using pip, but this is not listed in the instructions Suggestion: Intro for dummiesA brief abstract about how this package is useful for seismologists would be great for non seismologists like me. There is an hour youtube video available but that doesn’t work for someone scrolling through |
There's a couple of things that are still open.
All of the above is especially important if code is being pushed straight to master. I think the fixes are fairly easy which is why I'm mentioning this here. These two things will help potential new contributors from experiencing frustration! Hope this helps! |
|
Thanks for the comments on the review @megies, I think all the questions were answered well. Just the last thing I am not really sure about is the failing of CI tests (Travis fails: https://travis-ci.org/obspy/obspy). I agree with @canyon289 that this is rather confusing and could be fixed by allowing some tests to fail. I think many people use this to quickly evaluate a package for a) practical use / quality and b) ensure functioning during development of new features. In general, I would like to see the CI tests work for accepted packages in pyOpenSci.
The links to seismic live don't work for me, you might want to check those. Readme looks good now.
I agree with @megies, I think this is fine as is. @lwasser I feel PyOpenSci should leave the exact implementation to the developers and not mandate that these vignettes should be in the API. It is nice, but for some applications it is not feasible in my opinion.
I was indeed talking about the Pypi badges, great! |
I did not see the problem with the developer installation instructions in the wiki, to be honest. It was advertising using Anaconda as the most straightforward solution, in which case the mentioned error is not encountered as numpy will be there. For people not following the recommendation, I would expect them to be experienced and I would deem "No module named numpy. Please install numpy first, it is needed before installing ObsPy." as sufficiently clear what is the problem. In any case, it never hurts to further improve so I made some more changes, which should be crystal clear now.
I am sorry, but I don't see this being feasible right now in a manageable amount of time or without being disruptive to the whole test setup. Tests that can fail locally in maybe even 50% of the test cases, depending on installed numpy, scipy, matplotlib, ... version, depending on operating system in some cases, depending on platform and depending on network availability, etc etc. To be honest, it is even easier to keep false positive fails under control in CI as opposed to user installations (esp. because of volatile dependencies). I doubt it will ever be possible to a-priori identify those at all. Also, pytest is not (yet) our main testing backend but rather plain unittest with our test runner on top, steering things. In any case, thanks again for the review comments! |
You mean http://seismo-live.org/ is not working for you? Maybe a random network fail. The link should work
Looking at the earthpy vignette gallery @lwasser linked, actually this is pretty much what our gallery is? It also has a mosaic of plots that link to corresponding parts of the API pages. I can see that there is some argument about whether green badges are more important or being able to tell when things really go wrong, and when they started to do so (from the database of test results), and many people are speaking out for skipping tests or removing them -- I don't like either. I think this can not be resolved short term for this review, if anything, I'd rather remove those badges from the README, to be honest. We have a lot of tests relying on network/server feedback, image tests and numerical tests, all of which can be hard to keep passing with volatile dependencies, across platforms and external upstream server and network issues. I really don't see the point of a green badge when it mostly tells you "we removed all failing tests from the code base or just don't run em anymore because they just were a pain". |
|
hey there @megies !! this is great insight - thank you for it. I don't want these issues to STOP this review process! what has been happening is that every review we learn something and potentially change, enhance update our docs. We are new and want to ensure our review process is
So what we are trying to do is find that middle ground where the python scientific community says "cool - we think pyopensci guidelines are ones we want to follow because they are reasonable yet still robust". We (including @choldgraf ) had a great discussion about tests and expectations for pyopensci in our last meeting. I absolutely agree that we won't resolve this - in this review. We have some other ongoing discussions about how much documentation is "good enough". I'd like to find a middle ground for this review and then add what we learn to our docs. i will followup on language in our dev guide that might address this and would really appreciate your input on this @megies because as a developer of a larger package, i want to ensure things aren't too onerous for you! We are running into the same issue with docs. in the most perfect ideal world we'd have docs as follows:
i see in our docs we ask for a get started in the readme, vignettes and doc strings which seems like a lot. we are trying to find a "good enough" middle ground. This is what we came to in our last meeting and i'm curious what you all think about it! i plan to add a bit more to our discourse discussion as well but let me know what you all think about below: i don't have any language for testing yet but would love some help editing our dev_guide language to ensure it's reasonable. I agree that just removing failing tests is not the answer if they do serve some purpose. i'm just not sure what language is best in this case. thank you all for your patience. developing our review guidelines has been challenging but also a lot of fun as we are learning so much and getting a lot of input from people like you guys! |
|
@megies @megies @lwasser and @choldgraf For what its worth I wrote a blog post on the thought process and our solution here. Not suggesting this is what PyOpenSci or obspy should be use as a standard. Just some more background on what my/our open source projects ended up doing, if it is at all helpful. |
|
@lwasser no worries and I hope everything got sorted out 👍 @lwasser This looks like a good starting point for a classification. From my experience it might be enough to say (for e.g. "better") But then I have to be self-critical here, obspy has grown from small to very big and some policies for better coding practice were picked up one by one. So at some point we set a policy that all functions/methods/classes deemed private are prefixed by one underscore, and subject to change of API or complete removal without warning or deprecation at devs discretion. For these I would also not demand full documentation or examples in most cases. But, we have lots of such functionality that is not marked private by naming, but we would still consider it more internal helpers that do not need full effort in docstrings or even examples. So I guess in our case you could say that it is not straight forward to tell what is really supposed to be deemed "user-facing". These are historical artifacts and may be similar (or not..) for other projects that were not laid out on the drafting table but rather have grown steadily. Also, I think many smaller, yet user-facing routines might not not full blown tutorials. As an example, this helper function is for users, but I would argue the docstring says it all: http://docs.obspy.org/master/packages/autogen/obspy.geodetics.base.gps2dist_azimuth.html |
|
ok team. My apologies for being absent for the past few weeks. So many things have been happening. getting back to this and reading through everything again, it seems like the reviewers are generally happy with obspy now. I see that @megies did make the install a bit more clear! thank you. it looks great and i see numpy mentioned as a requirement. So here we are left with two bigger issues --
@canyon289 and @raoulcollenteur have i represented things well above? are there other outstanding issues in this particular review? |
|
@lwasser Apologies on my late reply That list looks good. The one other thing I would add is whether the master branch of CI should be failing or not. Basically should there be a green badge for CI on the README or not. |
|
Good morning, @megies et al. I am following up on this review. I realize this is a long stale issue but I wanted to revisit to dot i's and cross t's as necessary. First, thank you all for your time in submitting and reviewing this package. If i recall, the issue with proceeding with this review was the fact that many of the tests (please correct me if i'm wrong) were failing due to understandable issues. But i think we, as a community need to talk more about what we look for in test suites. What is difficult from a contribution standpoint is that if tests regularly fail, it might be hard for contributors to understand the points of failure and contribute meaningfully. I'm happy to have a more in depth discussion about this @megies if you'd like to chat further as it could help us as we define standards for review in pyOpenSci. Please let me know. For now, I am going to close this issue. However, if anyone responds I am happy to reopen it and if we need to have a broader discussion, we can move it over to discourse for that purpose. |
lwasser
added
the
on-hold
Submitting Author: @megies
Package Name: ObsPy
One-Line Description of Package: A Python Toolbox for seismology/seismological observatories.
Repository Link: https://github.com/obspy/obspy
Version submitted: 1.1.1
Editor: @lwasser
Reviewer 1: @canyon289
Reviewer 2: @raoulcollenteur
Archive: TBD
Version accepted: TBD
Description
ObsPy is an open-source project dedicated to provide a Python framework for processing seismological data. It provides parsers for common file formats, clients to access data centers and seismological signal processing routines which allow the manipulation of seismological time series (see Beyreuther et al. 2010, Megies et al. 2011, Krischer et al. 2015).
The goal of the ObsPy project is to facilitate rapid application development for seismology.
ObsPy is licensed under the GNU Lesser General Public License (LGPL) v3.0.
Scope
* Please fill out a pre-submission inquiry before submitting a data visualization package. For more info, see this section of our guidebook.
It falls under these categories, because we retrieve and munge data. Also loads of the functionality is concerning geospatial aspects (can't be avoided in seismology). Being as high-level as we are in functionality, I'd also consider it to be educational, since you can illustrate how to do many processing steps (and what pitfalls there are) in a few lines of code.
If this is more about the core aspects of the software, I guess you could also argue for only
retrievalandmunging, and the other are rather side aspects.. although it's unclear to me in which bullet item signal processing is included (mungingI guess?).Mainly scientists/researchers, but we know that obspy is also used in industry (no idea about numbers, though). Scientific applications span most fields of seismology that involve real or synthetic data, basically only excluding purely theoretical, pen and paper seismology.
There are various packages with I/O of up to a handful file formats at a time, usually concerned only with 1-3 file formats used by the authors. I am not aware of other packages that offer access clients for all important protocols like we do (FDSN web services, SeedLink, Earthworm, Arclink, NRL, syngine, ...). Most important point would be the widespread use in projects built on top of obspy, most likely.
@tagthe editor you contacted:no pre-submission enquiry filed.
Technical checks
For details about the pyOpenSci packaging requirements, see our packaging guide. Confirm each of the following by checking the box. This package:
__init__.pys) hold the submodules' most important functions with explanations and usage examples,Publication options
We have citable papers out, so no need for this I think. If anything we might do a "history of obspy" short story?
JOSS Checks
paper.mdmatching 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
We're using Contributor Covenant Code of Conduct v1.4
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: