Submit pydov for review

Question

Submit pydov for review

stijnvanhoey opened this issue 5 years ago · 37 comments

Answer 1 · 2020-01-31T23:45:21.000Z

Hi @stijnvanhoey !!! 👋 thank you for this submission!! i will add this to our list of packages to discuss in our next meeting which is next week - feb 6!!

Answer 2 · 2020-02-06T18:39:23.000Z

@NickleDave will be a reviewer for this submission!! @lwasser will try to find a second reviewer and editor (or she will be editor)

Answer 3 · 2020-03-01T20:28:53.000Z

I would be happy to help here as a second reviewer :)

Answer 4 · 2020-04-07T15:25:40.000Z

hey @NickleDave and @xmnlab i know there is a lot going on. i just wanted to check in on this review to see how you guys were doing. I am very behind and know that ropensci is stopping new submissions for atleast a month. are you guys still able to review this package or should we take a step back?

Many thanks.

Answer 5 · 2020-04-07T17:17:20.000Z

I can work on that. I will start that this week.

Answer 6 · 2020-04-07T18:47:05.000Z

Same here, I think I can finish by next weekend at the latest.

Answer 7 · 2020-04-07T19:07:05.000Z

Thank you so much @xmnlab and @NickleDave !! much appreciated.

Answer 8 · 2020-04-07T19:13:02.000Z

ok let's shoot for both reviews in by Wed April 15th just in case you guys need a bit more time!! and reach out please if you need anything.

Answer 9 · 2020-04-16T00:02:05.000Z

Hi @Roel @stijnvanhoey @pjhaest thank you for your patience. Please find below my review:

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

As the reviewer I confirm that there are no conflicts of interest for me to review this work (If you are unsure whether you are in conflict, please speak to your editor before starting your review).

Documentation

The package includes all the following forms of documentation:

A statement of need clearly stating problems the software is designed to solve and its target audience in README
Installation instructions: for the development version of package and any non-standard dependencies in README
Vignette(s) demonstrating major functionality that runs successfully locally
Function Documentation: for all user-facing functions
Examples for all user-facing functions
Community guidelines including contribution guidelines in the README or CONTRIBUTING.
Metadata including author(s), author e-mail(s), a url, and any other relevant metadata e.g., in a setup.py file or elsewhere.

Readme requirements
The package meets the readme requirements below:

Package has a README.md file in the root directory.

The README should include, from top to bottom:

The package name
Badges for continuous integration and test coverage, the badge for pyOpenSci peer-review once it has started (see below), a repostatus.org badge, and any other badges. If the README has many more badges, you might want to consider using a table for badges, see this example, that one and that one. Such a table should be more wide than high.
Short description of goals of package, with descriptive links to all vignettes (rendered, i.e. readable, cf the documentation website section) unless the package is small and there’s only one vignette repeating the README.
Installation instructions
Any additional setup required (authentication tokens, etc)
Brief demonstration usage
Direction to more detailed documentation (e.g. your documentation files or website).
If applicable, how the package compares to other similar packages and/or how it relates to other packages
Citation information

Functionality

Installation: Installation succeeds as documented.
Functionality: Any functional claims of the software been confirmed.
Performance: Any performance claims of the software been confirmed.
Automated tests: Tests cover essential functions of the package and a reasonable range of inputs and conditions. All tests pass on the local machine.
Continuous Integration: Has continuous integration, such as Travis CI, AppVeyor, CircleCI, and/or others.
Packaging guidelines: The package conforms to the pyOpenSci packaging guidelines.

For packages co-submitting to JOSS

The package has an obvious research application according to JOSS's definition in their submission requirements.

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:

A short summary describing the high-level functionality of the software
Authors: A list of authors with their affiliations
A statement of need clearly stating problems the software is designed to solve and its target audience.
References: with DOIs for all those that have one (e.g. papers, datasets, software).

Final approval (post-review)

The author has responded to my review and made changes to my satisfaction. I recommend approving this package.

Estimated hours spent reviewing: 3

Review Comments

At a high-level pydov is a fully-developed packages that meets the PyOpenSci review criteria. It provides impressive functionality, yet is tightly scoped to querying and downloading data from Databank Ondergrond Vlaanderen (DOV).

I have one request for the documentation, and one question about the code, although I don't think the question about the code needs to be addressed before approving the package.

The documentation is very thorough, but I think one thing that would help new users is an "intro to working with pydov" tutorial. This tutorial would explains things at a level between the quickstart and the very detailed tutorials for each individual search object. What this tutorial would provide, that the current tutorials do not have, is an introduction to the core abstractions provided by the package. I notice that each DOV search tutorial follows the same structure, but there is not a lot of language about why a potential user might want to use some of the functionality, e.g. why get all the groundwater screens / boring holes within a bounding box.

The intro tutorial would ideally be a specific example of an analysis--it could even be one of the existing search tutorials--that introduces the concepts that repeat throughout the individual tutorials for each search object. This intro should be targeted at a "naive" researcher who is interested in having access to the data, but might not be familiar with some of the existing data structures and libraries that pydov leverages, e.g. the WFS protocol. I admit to a total lack of domain expertise in this area, so the devs' idea of what a "naive" researcher would know compared to mine might vary, but I think that adding an intro with more language about about what and why the package does what it does, in addition to how to do those things, would make it more welcoming to new users.

My question about the code relates to the search sub-package. I understand the need for the types sub-package, because each class in that sub-package encapsulates the metadata such as wfs and xml fields specific to each dataset type. However it's not quite clear to me why different classes are needed for the search objects that correspond to each type in the search package. Am I right that all the search classes have the exact same attributes and methods? My read of the code is that the only difference between the search classes is the default value for objecttype. E.g., a BoringSearch class has its objecttype default =Boring , which dictates what fields it has, but aside from that, it has the exact same methods as all the other search types. If I'm misunderstanding something about the code, please let me know and I'll just remove this question. But it seems like it might be possible to reduce code repetition by having another class that inherits from AbstractSearch with all the code that is common to the specific Search classes, and then just subclass this additional class for specific types.
Something like:

class MetaSearch(AbstractSearch):
    def __init__(self, layer, objecttype):
        super(MetaSearch, self).__init__('dov-pub:Boringen', objecttype)

    def __common_search_methods_here__():
    ...

import Boring

BoringSearch(MetaSearch)
    def __init__(self):
        super(BoringSearch, self).__init__(layer='dov-pub:Boringen', objecttype=Boring)

Thank you again for your patience during this crazy time and thank you for submitting the package, it was a pleasure to review. Looking forward to your response

Answer 10 · 2020-04-16T04:39:32.000Z

hey everyone, I couldn't finish my review today. I will finish that tomorrow. sorry for the delay.

Answer 11 · 2020-04-16T17:41:41.000Z

@Roel, @stijnvanhoey, @pjhaest, thanks for your patience. pydov looks awesome, I am adding my review notes 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

As the reviewer I confirm that there are no conflicts of interest for me to review this work (If you are unsure whether you are in conflict, please speak to your editor before starting your review).

Documentation

The package includes all the following forms of documentation:

A statement of need clearly stating problems the software is designed to solve and its target audience in README
Installation instructions: for the development version of package and any non-standard dependencies in README
Vignette(s) demonstrating major functionality that runs successfully locally
Function Documentation: for all user-facing functions
Examples for all user-facing functions
Community guidelines including contribution guidelines in the README or CONTRIBUTING.
Metadata including author(s), author e-mail(s), a url, and any other relevant metadata e.g., in a setup.py file or elsewhere.

Readme requirements
The package meets the readme requirements below:

Package has a README.md file in the root directory.

The README should include, from top to bottom:

The package name
Badges for continuous integration and test coverage, the badge for pyOpenSci peer-review once it has started (see below), a repostatus.org badge, and any other badges. If the README has many more badges, you might want to consider using a table for badges, see this example, that one and that one. Such a table should be more wide than high.
Short description of goals of package, with descriptive links to all vignettes (rendered, i.e. readable, cf the documentation website section) unless the package is small and there’s only one vignette repeating the README.
Installation instructions
Any additional setup required (authentication tokens, etc)
Brief demonstration usage
Direction to more detailed documentation (e.g. your documentation files or website).
If applicable, how the package compares to other similar packages and/or how it relates to other packages
Citation information

Functionality

Installation: Installation succeeds as documented.
Functionality: Any functional claims of the software been confirmed.
Performance: Any performance claims of the software been confirmed.
Automated tests: Tests cover essential functions of the package and a reasonable range of inputs and conditions. All tests pass on the local machine.
Continuous Integration: Has continuous integration, such as Travis CI, AppVeyor, CircleCI, and/or others.
Packaging guidelines: The package conforms to the pyOpenSci packaging guidelines.

For packages co-submitting to JOSS

The package has an obvious research application according to JOSS's definition in their submission requirements.

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:

A short summary describing the high-level functionality of the software
Authors: A list of authors with their affiliations
A statement of need clearly stating problems the software is designed to solve and its target audience.
References: with DOIs for all those that have one (e.g. papers, datasets, software).

Final approval (post-review)

The author has responded to my review and made changes to my satisfaction. I recommend approving this package.

Estimated hours spent reviewing: 4 hours

Review Comments

Congratulation for the work you all are doing on pydov. I am adding general notes below:

The documentation looks very detailed and instructive about how to use the library. I agree with David, maybe would be good to have a tutorial with an introduction something like a blog post (also it could be posted on PyOpenSci blog section).
API reference documentation looks very well detailed.
Good test coverage (96%).

Requests

There is no instructions about how to install the package for development in README file (e.g. prepare the environment: "pip install -r requirements_dev.txt", install the package in the development mode "python setup.py develop", etc)
The quick start located at README maybe could be improved with the output of dataframe, so it will give more details about the result of a boring search. It also applies to https://pydov.readthedocs.io/en/stable/quickstart.html

Suggestions and general comments (not required)

Maybe it would be good to have a link to binder on each tutorial page to its corresponding tutorial on binder
In the tests, you can change assert type(df) is DataFrame to assert isinstance(df, DataFrame)
It is not too common to import objects from a test_*.py maybe it could be moved to conftest (if it applies)
It seems the test classes are inheriting from a base class (AbstractTestTypes, AbstractTestSearch) and some methods' docstring are the same as the inherited method, using this approach is very hard to maintain and keep everything consistent. Also some methods could be an attribute, e.g.: def get_type(self): return Boring -> klass = Boring and it keeps your test classes more compact.
The pydov classes are also inheriting from base classes, and the docstrings work looks duplicated, if you don't add any docstring, the method will inherit from the base clases. If you think you need to have a customized docstring, maybe you can check some approaches for dynamic docstrings
typo: https://pydov.readthedocs.io/en/stable/notebooks/search_grondwaterfilters.html#Plotting "... Pandas cen be used"
It would be good to have in each chart the title and label for each axis
Maybe pydocstyle can help to check static docstrings (you also can ignore erros you think any one doesn't apply for pydov). If you want to check dynamic docstrings you can use numpydoc --validate (https://numpydoc.readthedocs.io/en/latest/validation.html#validating-numpydoc-docstrings) you can check one example here https://github.com/pandas-dev/pandas/blob/master/scripts/validate_docstrings.py

Answer 12 · 2020-04-18T15:56:00.000Z

Hi @NickleDave @xmnlab

Thank you for taking the time to review our package so thouroughly during these strange times.

All the points you raised are valid and I have made issues for them in our Github project so we can track progress there as we work ourselves through them. We plan to have a two-day codesprint somewhere in May, and I'm confident we can tackle most of the issues then.

@NickleDave: I agree that an introductory tutorial that introduces the search concepts more in detail (and by example) would help new users. As you mention the concepts are the same for each of the search objects, so if a new user can understand these concepts from the introductory tutorial they can quickly move on to the subject of their interest. We will definitely work on this in the codesprint.

Regarding your question about the search classes, you are right that each of them duplicates quite a lot of code. The idea is to store the (results of the remote calls to the) WFS schema, metadata, XSD schemata and so on in class variables of the search classes so that they are shared between search class instances. This avoids dowloading this data multiple times in the same session. There might be a better way to achieve this though. Another option we can consider is letting go this behaviour of keeping this data in class variables in the first place. Since the search instances don't persist data between searches there should be no need to create more than one. I made an issue for this in our Github, we can discuss this further either there or here, as you please.

@xmnlab: We will add installation instructions for a development installation in the README as well as in the installation section in the documentation. There are bits and pieces of information (f.ex. in the contributing section), but that is quite hard to find and assemble :)

We will also add the output dataframe of the search, both in the README as well as in the Quick start in the documentation. I agree that (potential) users can so quickly see when they can get as output.

I have made issues for your other suggestions. Thanks very much for tips, we will definitely look into them!

Thanks a lot to both of you for your time and reviews! We will work through your suggestions as time allows during the next couple of weeks and will follow up here once we made some progtess :)

Stay safe!

Answer 13 · 2020-04-20T16:38:32.000Z

hi @Roel @xmnlab @NickleDave thank you all for your work on this review! @Roel if and when you have time, can you kindly reference this issue in your issues / pr's as you work through them? this will make it easier to circle back on the review in may or june (or whatever timing works for you) to ensure things have been resolved!! Many thanks all.

Answer 14 · 2020-05-22T21:49:59.000Z

👋 all!! just checking in here... it looks like there are a bunch of issues and such being worked through still so that is totally cool. if anyone has questions please say the word... otherwise i'll just wait to see things closed up and then will check in again to see whether the reviewers are satisfied!! i'll be offline next week so i won't be responsive until june 1! Stay healthy everyone :)

Answer 15 · 2020-05-24T15:59:06.000Z

Hi @lwasser ! Our code sprint is planned for next week (May 28-29), so I hope to make some progress in the issues then. I assume we should be able to pick up the review somewhere in June :)

Thanks for your patience, enjoy your week offline and stay healthy!

Answer 16 · 2020-06-03T19:25:44.000Z

@Roel sounds great! just ping us here when you are ready to pick back up. i know this all takes a lot of time so take the time that you need!

Answer 17 · 2020-07-27T17:31:22.000Z

Hi all! I hope everyone's doing well out there. We managed to tackle most of the raised points during our last code sprint and in the weeks that followed. There are still two issues unresolved for now, but we'll take these on in one of our next sprints for sure.

Let me know whether it suits you to pick back up here. Thanks for you patience and take care!

Answer 18 · 2020-10-14T15:50:31.000Z

hi all. just checkin in to see whether all review comments have been addressed or who is working on what items at this point. many thanks.

Answer 19 · 2020-10-14T22:28:03.000Z

oh sorry, I think I lost this notification, really sorry. I will take a look at the last comment @Roel sent. it sounds pretty nice that a lot of issues were addressed at the sprint! that is great :)

Answer 20 · 2020-10-21T00:41:56.000Z

@Roel thank you so much for your patience! thanks for addressing the points I suggested.

@lwasser, The author has responded to my review and made changes to my satisfaction. I recommend approving this package.

Answer 21 · 2020-10-21T14:59:05.000Z

thank you @xmnlab !!! @NickleDave you were the second reviewer on this package. have all changes been implemented per your satisfaction? it sounds like there are two outstanding @Roel can you tell us what is left to do following this review from your perspective? and perhaps at this point some have been addressed in a sprint?

Answer 22 · 2020-10-21T15:14:39.000Z

Thank you for pinging me @lwasser -- sorry for losing track of this

Yes I see that DOV-Vlaanderen/pydov#254 adds an introductory tutorial -- looks good @Roel, I think this can definitely help a newcomer quickly get up to speed on usage of pydov.

The other concern I raised about code duplication has been noted in DOV-Vlaanderen/pydov#264 but I don't think this should be a blocking issue.

So, yes, the authors have responded and made changes to my satisfaction.
I recommend approving pydov (🎉)

Answer 23 · 2020-10-26T18:28:15.000Z

Awesome, this is great news! No worries about the timing, we're all busy :)

We will definetely pick up the remaining issues in one of our next sprints and keep them in mind when refactoring or working on new features.

Thank you again for your time @lwasser, @xmnlab and @NickleDave! Great to become a part of the growing list of pyOpenSci packages!

Answer 24 · 2020-10-27T02:20:58.000Z

Thank you for submitting pydov for review @Roel @pjhaest @stijnvanhoey !!
I think it's a great tool, excited to tell more people about it.
Perfect for pyOpenSci. And I learned a lot from seeing you all develop it further.

Answer 25 · 2020-10-28T15:05:43.000Z

🎆 excellent! ok i think then we can consider pydov accepted as a part of pyopensci!!
thank you @Roel @pjhaest @stijnvanhoey and @NickleDave @xmnlab for the efficient review process here!!

there are a few last steps if someone is willing to take the time to do this

Reviewers:

If you are not already on our website as a contributor, please submit a PR to pyopensci.github.io to add yourself as a contributor using:

contributor_type:
    - reviewer

Package Maintainer(s)

@stijnvanhoey for you the tasks are:

Add the badge for pyOpenSci peer review, with the second link being the URL to this issue:
Submit a PR to pyopensci.github.io to add pydov to the pyOpenSci package list and to add yourself as a contributor using:

contributor_type:
    - package-maintainer

and

packages-submitted: ["pydov"]

OPTIONAL!!

If you are interested (not required), you can write a blog post for the pyOpenSci website about PyDov (see blog post about pandera) to promote your package. This is totally up to you!

Is the version that we are accepting here 2.0.0 ?
Thank you all!!

Answer 26 · 2020-10-28T16:59:54.000Z

This is indeed great news, thanks reviewers for all the comment and all credit to @Roel for being the main driver to get these changes and improvement all tackled. Thanks a lot!

With respect to the badge, see DOV-Vlaanderen/pydov#292

Answer 27 · 2021-01-11T21:43:35.000Z

Hi there @stijnvanhoey ! i'm doing some housekeeping today and i noticed pydov is not on our list of packages here. Nor are you listed as a contributor! did a PR get missed? i don't see one but that does not mean i didn't miss something. we can close this once this step is complete! i hope you are well!

Answer 28 · 2021-01-15T23:00:29.000Z

Hi @lwasser, thanks for the update! I don't know what we missed. But we are having a next code sprint in some weeks. If it's not picked up before that time, we'll definitely do it at that occasion.
Thx all!

Answer 29 · 2021-03-04T00:25:13.000Z

Closing this--thank you again all for a great review process!

Answer 30 · 2022-10-19T20:22:43.000Z

hi there @stijnvanhoey !! i wanted to check in on pydov here! pyOpenSci has funding to operate for a few years now and i'm focused on the project.

As a first step, i'm reaching out to all maintainers and reviewers who have worked with us in a review to request that they complete a survey:

🔗 Link here
this should take you about 10 to 15 minutes and i'd greatly appreciate your time in doing it.

Also are you the only maintainer of this package? If there are other maintainers can you please list them here and kindly ask them to take the survey as well?

We also have a slack community that you are welcome to join if you'd like. i can email you with a link.

Many thanks in advance for doing this! And i hope all is well!

Answer 31 · 2022-10-19T20:24:00.000Z

actually i think there are three maintainers possible here?
@Roel , @pjhaest I just pinged @stijnvanhoey above but would appreciate your filling our our🔗 SURVEY as well. and you are also welcome to join us on slack. Many thanks again for your time.

Answer 32 · 2022-10-28T13:34:50.000Z

Hey there - @Roel, @stijnvanhoey, @pjhaest just checking in again here to make sure you recieved my message above! It's important that packages in our pyOpenSci ecosystem are maintained and that we have some level of communication with our maintainers. I think pydov is still being maintained.

But i'd like to know who the current maintainers are so please do get back to me here on this issue. And we'd GREATLY appreciate your time in filling out this survey as well.

Many thanks for your time in doing these things! 🙌

Answer 33 · 2022-10-28T20:50:51.000Z

@lwasser thanks for reaching out. @Roel is indeed the current maintainer and doing all of the updates and new releases. Would be best to have @Roel filling in the survey.

Answer 34 · 2022-10-29T08:59:42.000Z

Hi @lwasser

I'm indeed doing most of the maintenance of pydov currently. We had a bugfix release just a few days ago and a new major release is coming up soon!

I hope to find the time next week to fill in the survey.

Answer 35 · 2022-10-31T12:27:37.000Z

ok awesome. @Roel i'll list you as the lead maintainer then for this package. Many thanks @stijnvanhoey @Roel for getting back to me!! and thanks in advance for working on the survey @Roel . I appreciate your time!

Answer 36 · 2022-11-03T15:46:31.000Z

Hi @lwasser I filled in the survey, I hope the results will be useful for you!

Answer 37 · 2022-11-03T19:11:25.000Z

@Roel thank you SO SO MUCH for that!! i appreciate it.

Submit pydov for review

Description

Scope

Technical checks

Publication options

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

Code of conduct

Editor and Review Templates

Package Review

Documentation

Functionality

For packages co-submitting to JOSS

Final approval (post-review)

Review Comments

Package Review

Documentation

Functionality

For packages co-submitting to JOSS

Final approval (post-review)

Review Comments

Reviewers:

Package Maintainer(s)