Skip to content
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

automata #152

Open
15 of 30 tasks
eliotwrobson opened this issue Dec 31, 2023 · 38 comments
Open
15 of 30 tasks

automata #152

eliotwrobson opened this issue Dec 31, 2023 · 38 comments

Comments

@eliotwrobson
Copy link

eliotwrobson commented Dec 31, 2023

Submitting Author: Eliot Robson (@eliotwrobson)
All current maintainers: (@eliotwrobson, @caleb531)
Package Name: automata
One-Line Description of Package: A Python library for simulating finite automata, pushdown automata, and Turing machines.
Repository Link: https://github.com/caleb531/automata
Version submitted: v8.2.0
EiC: @isabelizimm
Editor: @sneakers-the-rat
Reviewer 1:
Reviewer 2:
Reviewers (@pyosreviewer1, @pyosreviewer2): (@phildong , @irisdyoung, @NimaSarajpoor)
Reviews Expected By: March 28th, 2024
Archive: TBD
JOSS DOI: TBD
Version accepted: TBD
Date accepted (month/day/year): TBD


Code of Conduct & Commitment to Maintain Package

Description

  • Include a brief paragraph describing what your package does:

Automata is a Python 3 library implementing structures and algorithms for manipulating finite automata, pushdown automata, and Turing machines. The algorithms have been optimized and are capable of processing large inputs. Visualization logic has also been implemented.

Scope

  • Please indicate which category or categories.
    Check out our package scope page to learn more about our
    scope. (If you are unsure of which category you fit, we suggest you make a pre-submission inquiry):

    • Data retrieval
    • Data extraction
    • Data processing/munging
    • Data deposition
    • Data validation and testing
    • Data visualization1
    • Workflow automation
    • Citation management and bibliometrics
    • Scientific software wrappers
    • Database interoperability

Domain Specific & Community Partnerships

- [ ] Geospatial
- [x] Education
- [ ] Pangeo

Community Partnerships

If your package is associated with an
existing community please check below:

  • For all submissions, explain how the and why the package falls under the categories you indicated above. In your explanation, please address the following points (briefly, 1-2 sentences for each):

    • Who is the target audience and what are scientific applications of this package?

This package is suitable for both researchers wishing to manipulate automata and for instructors teaching courses on theoretical computer science. Automata (especially finite automata) are important models in computing that appear in a variety of educational and research contexts, and the ability to manipulate them with this package is valuable to this effort.

  • Are there other Python packages that accomplish the same thing? If so, how does yours differ?

There are some smaller packages with similar scope (for example here), but automata is the most popular, best maintained, and most feature-rich.

  • If you made a pre-submission enquiry, please paste the link to the corresponding issue, forum post, or other discussion, or @tag the editor you contacted:

#135

  • Any other questions or issues we should be aware of:

Although I am not the primary repository owner (that is @caleb531), he has given me permission to make this submission and be a long-term point of contact as-needed. Additionally, our documentation page is brand-new, so we anticipate some rough edges, and hope that feedback from this review can be used to improve the usability and add examples. Automata also already appeared in JOSS, so that is why those items have been left blank here even though the writeup is present in the repository.

Technical checks

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

  • does not violate the Terms of Service of any service it interacts with.
  • uses an OSI approved license.
  • contains a README with instructions for installing the development version.
  • includes documentation with examples for all functions.
  • contains a tutorial with examples of its essential functions and uses.
  • has a test suite.
  • has continuous integration setup, such as GitHub Actions CircleCI, and/or others.

Publication Options

JOSS Checks
  • The package has an obvious research application according to JOSS's definition in their 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.
  • The package is not a "minor utility" as defined by JOSS's 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.
  • The package contains a paper.md matching JOSS's requirements with a high-level description in the package root or in inst/.
  • The package is deposited in a long-term repository with the DOI:

Note: JOSS accepts our review as theirs. You will NOT need to go through another full review. JOSS will only review your paper.md file. Be sure to link to this pyOpenSci issue when a JOSS issue is opened for your package. Also be sure to tell the JOSS editor that this is a pyOpenSci reviewed package once you reach this step.

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.

  • Yes I am OK with reviewers submitting requested changes as issues to my repo. Reviewers will then link to the issues in their submitted review.

Confirm each of the following by checking the box.

  • I have read the author guide.
  • I expect to maintain this package for at least 2 years and can help find a replacement for the maintainer (team) if needed.

Please fill out our survey

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

Editor and Review Templates

The editor template can be found here.

The review template can be found here.

Footnotes

  1. Please fill out a pre-submission inquiry before submitting a data visualization package.

@lwasser
Copy link
Member

lwasser commented Jan 5, 2024

hey there @eliotwrobson 👋 Welcome to pyOpenSci!! I just wanted to let you know that we have seen this submission and will be following up in the next week about moving forward with it! Have a wonderful weekend. ✨

@lwasser lwasser self-assigned this Jan 5, 2024
@lwasser
Copy link
Member

lwasser commented Jan 18, 2024

hi again @eliotwrobson @caleb531 👋 my apologies for the delay! we are making a few changes to our editorial process and that took a bit of time to sort out!

Our Editor in Chief for the next quarter, @isabelizimm will be running initial checks on this package in the upcoming week. After that @sneakers-the-rat will lead the review as editor! You will hear from our editorial team sometime in the next week to kick off the review. ✨ Thank you for your patience!

@isabelizimm
Copy link
Contributor

Thank you so much for this submission! I've done some preliminary checks below, which all look great 🎉 your editor will be @sneakers-the-rat, who will take it from here with finding reviewers.

Editor in Chief checks

Hi there! Thank you for submitting your package for pyOpenSci
review. Below are the basic checks that your package needs to pass
to begin our review. If some of these are missing, we will ask you
to work on them before the review process begins.

Please check our Python packaging guide for more information on the elements
below.

  • Installation The package can be installed from a community repository such as PyPI (preferred), and/or a community channel on conda (e.g. conda-forge, bioconda).
    • The package imports properly into a standard Python environment import package.
  • Fit The package meets criteria for fit and overlap.
  • Documentation The package has sufficient online documentation to allow us to evaluate package function and scope without installing the package. This includes:
    • User-facing documentation that overviews how to install and start using the package.
    • Short tutorials that help a user understand how to use the package and what it can do for them.
    • API documentation (documentation for your code's functions, classes, methods and attributes): this includes clearly written docstrings with variables defined using a standard docstring format.
  • Core GitHub repository Files
    • README The package has a README.md file with clear explanation of what the package does, instructions on how to install it, and a link to development instructions.
    • Contributing File The package has a CONTRIBUTING.md file that details how to install and contribute to the package.
    • Code of Conduct The package has a CODE_OF_CONDUCT.md file.
    • License The package has an OSI approved license.
      NOTE: We prefer that you have development instructions in your documentation too.
  • Issue Submission Documentation All of the information is filled out in the YAML header of the issue (located at the top of the issue template).
  • Automated tests Package has a testing suite and is tested via a Continuous Integration service.
  • Repository The repository link resolves correctly.
  • Package overlap The package doesn't entirely overlap with the functionality of other packages that have already been submitted to pyOpenSci.
  • Archive (JOSS only, may be post-review): The repository DOI resolves correctly.
  • Version (JOSS only, may be post-review): Does the release version given match the GitHub release (v1.0.0)?

  • Initial onboarding survey was filled out
    We appreciate each maintainer of the package filling out this survey individually. 🙌
    Thank you authors in advance for setting aside five to ten minutes to do this. It truly helps our organization. 🙌

@lwasser
Copy link
Member

lwasser commented Jan 19, 2024

hi there @eliotwrobson 👋 i wanted to point out two small items that you could address associated with your package's structure!

  1. you have a shell setup.py file here that you don't need! It did used to be the case that the setup.py file was needed for editable installs. HOwever that is no longer the case. You can remove it and all should be fine.
  2. Setuptools also installs wheel at runtime as a requirement. so you can remove wheel from your pyproject.toml build declaration

These are small changes but will modernize / clean up your package just a bit! :)

@eliotwrobson
Copy link
Author

@sneakers-the-rat are there any more action items we can handle before reviews start? Just wanted to check in, thanks!

@sneakers-the-rat
Copy link
Collaborator

Hello! sorry for the delay, i've been prepping for/at a workshop the last few weeks, but let's get this rolling. I have one reviewer that has previously agreed to review this, and am putting the call out for more.

@phildong
Copy link

Hello this is the "one reviewer" popping in and saying Hi. Very excited for this and ready to jump in whenever you're ready!

@sneakers-the-rat
Copy link
Collaborator

Hello and thank you for agreeing to review @phildong !!!!!

Still casting about for reviewers, I see @nathanael-fijalkow and @rohaquinlop as having worked on similar projects in the past by searching through github a bit, although they have not agreed to review for pyopensci, will ask again on masto and in reviewer channels :)

@sneakers-the-rat
Copy link
Collaborator

Checking in again, I'll make another call for a second reviewer, but @eliotwrobson if you could recommend anyone that might be suitable as a reviewer that would be great!

@eliotwrobson
Copy link
Author

@sneakers-the-rat thanks for checking in! I don't have anyone specific in mind (I'm honestly not sure what the qualifications are for review). If you wanted to, you could ask people that did the previous JOSS review? That might make things easier since they already know their way around.

@sneakers-the-rat
Copy link
Collaborator

We have located a second reviewer! Thank you to @irisdyoung for volunteering, and thanks @phildong for your patience :)

Now that we have two reviewers, let's get started!

pyOpenSci is a young-ish organization and doesn't have some of the fancier review bot features set up like JOSS yet (we're working on it!), but the reviews work similarly if you've done that before:

First, check out the reviewer guide here - https://www.pyopensci.org/software-peer-review/how-to/reviewer-guide.html (and you can raise issues if anything is unclear here: https://github.com/pyOpenSci/software-peer-review ). There are example reviews linked from there, and you can also see previous reviews i have done for pyopensci (pygraphblas) or joss (reorient)

My role here is to make sure the reviewers have everything needed to complete the review, and make sure the vibes stay close to a supportive, collaborative discussion. So I don't end up typing this 50 times: if you need anything at all, don't hesitate to ask. If you would prefer speaking privately, you can reach me on the fediverse or via email.

As an overview, you are going to do three things

  • Checklist - the review checklist serves as a set of minimal requirements for a package. The checklist is constructed by the editorial team, with input from lots of parts of the Python world, but if any parts of it are unclear (or you have any other thoughts on them) that can be up for discussion as well.
  • Discussion - As you go, you should raise issues or make pull requests with questions, comments, and suggestions. Since github issues don't have threading, we use issues/PRs as threads to keep discussions somewhat tidy. When you do so, please link back to this issue so that we can keep track of all the threads related to this review. Beyond that it is up to y'all if you want to have any sort of naming conventions, etc. for issues/PRs. Discussion happens throughout the review process, rather than only at the end! The authors are welcome to address and improve the package throughout the review as well.
  • Summary - At the end, you will write a summary of your thoughts on the package - its strengths, applicability, and potential points of future improvement.

As reviewers, you are taking the role of an interested and critical potential user - it should be possible for you to (non-exhaustively) a) understand what the package is supposed to be able to do, b) know how to do it, c) be able to do it, and d) be confident that functionality won't break. All that should require no special information from the authors - you are reviewing both for technical correctness as well as quality of documentation and technical infrastructure like CI. A decent heuristic is, by the end of the review, the package should be one that you would want to add as a dependency in your work.

The checklist is a minimum bar, but you are welcome to review anything beyond that - as reviewers you can coordinate amongst yourselves if one or the other of you has a preferred focus, eg. someone prefers reviewing for docs and the other wants to review for performance, etc. You can signal if a particular issue is a blocker for you or not, and we can resolve those as they come up - optional suggestions, comments, questions, etc. are also in bounds.

This is an open, collaborative review process - the checklist is about bringing packages in python world up to a minimal standard of maintainability and functionality, but it is not a gate to keep. There is no concept of "rejection" here, instead we are trying to help the authors reach that standard. Be the code reviewer you always wished you had <3.

And with that lengthy explanation... let's get started. the first thing you'll both want to do is copy and paste the review template (also added in the collapsible below) into a new comment that you can then edit as you do your review :) glhf <3

Let's set a tentative date to expect reviews 3 weeks from now - March 28th

Expand/collapse review template
## 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 the 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 `pyproject.toml` file or elsewhere.

Readme file  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,
    - [ ] Docs building (if you have a documentation website),
    - [ ] A [repostatus.org](https://www.repostatus.org/) badge,
    - [ ] Python versions supported,
    - [ ] Current package version (on PyPI / Conda).

*NOTE: If the README has many more badges, you might want to consider using a table for badges: [see this example](https://github.com/ropensci/drake). Such a table should be more wide than high. (Note that the a badge for pyOpenSci peer-review will be provided upon acceptance.)*

- [ ] Short description of package goals.
- [ ] Package installation instructions
- [ ] Any additional setup required to use the package (authentication tokens, etc.)
- [ ] Descriptive links to all vignettes. If the package is small, there may only be a need for one vignette which could be placed in the README.md file.
    - [ ] Brief demonstration of package usage (as it makes sense - links to vignettes could also suffice here if package description is clear)
- [ ] Link to your documentation website.
- [ ] If applicable, how the package compares to other similar packages and/or how it relates to other packages in the scientific ecosystem.
- [ ] Citation information

#### Usability
Reviewers are encouraged to submit suggestions (or pull requests) that will improve the usability of the package as a whole.
Package structure should follow general community best-practices. In general please consider whether:

- [ ] Package documentation is clear and easy to find and use.
- [ ] The need for the package is clear
- [ ] All functions have documentation and associated examples for use
- [ ] The package is easy to install


#### 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:**
  - [ ] All tests pass on the reviewer's local machine for the package version submitted by the author. Ideally this should be a tagged version making it easy for reviewers to install.
  - [ ] Tests cover essential functions of the package and a reasonable range of inputs and conditions.
- [ ] **Continuous Integration:** Has continuous integration setup (We suggest using Github actions but any CI platform is acceptable for review)
- [ ] **Packaging guidelines**: The package conforms to the pyOpenSci [packaging guidelines](https://www.pyopensci.org/python-package-guide).
    A few notable highlights to look at:
    - [ ] Package supports modern versions of Python and not [End of life versions](https://endoflife.date/python).
    - [ ] Code format is standard throughout package and follows PEP 8 guidelines (CI tests for linting pass)

#### For packages also submitting to JOSS

- [ ] The package has an **obvious research application** according to JOSS's definition in their [submission requirements](http://joss.theoj.org/about#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](http://joss.theoj.org/about#paper_structure) 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:

---

#### Review Comments

@sneakers-the-rat
Copy link
Collaborator

Checking in with reviewers, @phildong @irisdyoung - do we think we'll need more time?

@phildong
Copy link

@sneakers-the-rat sorry I was traveling last week. I aim to finish the review this week/weekend!

@phildong
Copy link

phildong commented Mar 23, 2024

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 the 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 pyproject.toml file or elsewhere.

Readme file 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,
    • Docs building (if you have a documentation website),
    • A repostatus.org badge,
    • Python versions supported,
    • Current package version (on PyPI / Conda).

NOTE: If the README has many more badges, you might want to consider using a table for badges: see this example. Such a table should be more wide than high. (Note that the a badge for pyOpenSci peer-review will be provided upon acceptance.)

  • Short description of package goals.
  • Package installation instructions
  • Any additional setup required to use the package (authentication tokens, etc.)
  • Descriptive links to all vignettes. If the package is small, there may only be a need for one vignette which could be placed in the README.md file.
    • Brief demonstration of package usage (as it makes sense - links to vignettes could also suffice here if package description is clear)
  • Link to your documentation website.
  • If applicable, how the package compares to other similar packages and/or how it relates to other packages in the scientific ecosystem.
  • Citation information

Usability

Reviewers are encouraged to submit suggestions (or pull requests) that will improve the usability of the package as a whole.
Package structure should follow general community best-practices. In general please consider whether:

  • Package documentation is clear and easy to find and use.
  • The need for the package is clear
  • All functions have documentation and associated examples for use
  • The package is easy to install

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:
    • All tests pass on the reviewer's local machine for the package version submitted by the author. Ideally this should be a tagged version making it easy for reviewers to install.
    • Tests cover essential functions of the package and a reasonable range of inputs and conditions.
  • Continuous Integration: Has continuous integration setup (We suggest using Github actions but any CI platform is acceptable for review)
  • Packaging guidelines: The package conforms to the pyOpenSci packaging guidelines.
    A few notable highlights to look at:
    • Package supports modern versions of Python and not End of life versions.
    • Code format is standard throughout package and follows PEP 8 guidelines (CI tests for linting pass)

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:


Review Comments

The automata package allow the users to easily build and manipulate finite automata, pushdown automata, and Turing machines. It's an invaluable tool for teaching and demonstration of this topic. The algorithm is also optimized for large inputs so it can be useful in theoretical research. The code is well-packaged with detailed documentation and CI setup. I was able to easily install and use the package.

Specific comments:

@phildong
Copy link

ok finished my first pass of review. @sneakers-the-rat since it's first time I do this please let me know if I'm on the right track and if there's any major stuff I'm missing!

Meanwhile I got some questions:

  • Am I supposed evaluate the novelty/impact the package? I can certainly see the package being a very nice resource to the community exactly as described, but it's a bit hard for me to compare it to any other potentially similar packages since I'm not expert in this field.
  • I see the "JOSS publication" section was not checked in the original submission, so do I still need to worry about the corresponding section in the review template?
  • The package has nice API documentation for all user-facing functions, and as someone who had to google "what's DFA" in the beginning, I could fairly easily figure out how to actually use the basic functions. That said, certainly not every function has usage example in the documentation. So I'm wondering what's the criteria for examples and what's the best way to proceed?

@eliotwrobson
Copy link
Author

@sneakers-the-rat (cc @irisdyoung) do you think we might be able to close out reviews soon? The end of the academic year is coming up for me, and it would be awesome to be able to close out the remaining items for this review before the start of the summer.

@sneakers-the-rat
Copy link
Collaborator

Apologies, have been afk and traveling for the last few weeks. Back at my desk tomorrow and yes lets scoot this along :)

@sneakers-the-rat
Copy link
Collaborator

sneakers-the-rat commented Apr 20, 2024

OK - checking in with @irisdyoung offline, @phildong is finished with first pass of review, so for that we'd be waiting on any final issues to raise and otherwise just a final review statement that summarizes a) outcomes of review checklist, b) overall impressions of package, c) strengths, and d) opportunities for growth :). I'll check in with phil separately monday if he's not checking on his gh notifications.

edit: will follow up with another timeline after making contact with reviewers

@sneakers-the-rat
Copy link
Collaborator

Checking in again - @irisdyoung any estimate on review time? and @phildong for some summary statement on the package if you have seen enough of it?

@eliotwrobson
Copy link
Author

@sneakers-the-rat just a heads up that I'm going to get started on the existing open issues from this review pretty soon, along with expanding the examples in the documentation (based on the unchecked boxes left in the open review).

@phildong
Copy link

Sorry for the silence!
I have followed up in the individual issues/PRs to provide some clarification (hopefully).
Once they're addressed/closed I'm happy to write up a summary and make that final tick 😄

@sneakers-the-rat
Copy link
Collaborator

@irisdyoung last call for review :) no worries if things are too hectic on your end, just want to know to start looking for another reviewer.

@irisdyoung
Copy link

Hello and apologies for the radio silence over here. I've done a first look through and can plan to raise any issues before the weekend. Talk soon!

@eliotwrobson
Copy link
Author

Docs PR addressing most of the currently open issues from this review: caleb531/automata#217

Basically adds discussion addressing most of the points of confusion that have come up so far.

Regarding more self-contained tutorials, that's coming up soon 👍👍

@sneakers-the-rat
Copy link
Collaborator

Great @irisdyoung thanks for letting us know.

@eliotwrobson
Copy link
Author

@irisdyoung any updates? I think we've addressed all the issues you opened.

@lwasser
Copy link
Member

lwasser commented Jun 6, 2024

hi there team 👋🏻 I wanted to check in on this review. your fearless editor @sneakers-the-rat is focused on other more pressing issues now going on in their professional environment. As such i'm just checking in to see if we can support Jonny in moving this review forward while they focus on bigger more important things.

As such, i'm trying to figure out the state of this review!
i see one review submitted from @phildong -- thank you!!
@irisdyoung i suspect you submitted some issues around the package but i don't see your format review submission using this template (please excuse me if i missed it!! )

it's important to have a tracebable record of the review so we ask that you both submit a formal review as a comment using that template and if you open issues / pr's (amazing!!) please link them back to this issue for traceability!

in the meantime I am also going to look for a pinch hitter editor to support this review to keep things moving forward.

everyone - please let me know how this sounds / if you have questions, concerns, etc / if i missed something!! ✨

thank you all for supporting our peer review process.

@eliotwrobson
Copy link
Author

eliotwrobson commented Jun 6, 2024

@lwasser thanks for checking in!! I think your assessment of the state of this review is correct. We've been trying to close all of the issues related to this review in a timely manner. There's one outstanding we may push back on somewhat given the effort involved and how long this review has been open, but otherwise there haven't been a ton of requests made by reviewers. With some support I think this can be pushed to close fairly soon (hopefully).

@lwasser
Copy link
Member

lwasser commented Jun 6, 2024

ok great.

  1. you are welcome to push back and we can discuss . I think the importance of the fix relative to it's effort involved is more important than how long the review takes. As our goal is really that the package works, is usable etc if that makes sense. But we also want to ensure the reviews aren't overly imposing on an author / maintainer! so let's discuss.
  2. we are looking for another editor to help get this review over the finish line
  3. it will be important for @irisdyoung to submit a formal review here. because as it stands, when looking at the review it appears to only have one review (while i do understand issues have been opened). we do need that review template filled out so we have a formal record of two reviews! 👐🏻

we will get things wrapped up here @eliotwrobson let's work together on getting all three of the above elements sorted out!

@eliotwrobson
Copy link
Author

In full agreement on 2 and 3. As for 1, in a previous comment, @phildong mentioned that he would be fine signing off his review once all the linked issues / PRs were closed: #152 (comment)

There is only one outstanding item from this review, and this is a request for instructional examples using the package. We're working on this currently (as we have actually wanted to add something self-contained like this for a while), and we'll definitely have this added at some point, but my only pushback is whether this should be an item blocking acceptance. Since we're still waiting on other things, this isn't really an issue yet, but something to keep in mind for later on.

@lwasser
Copy link
Member

lwasser commented Jun 7, 2024

ahhh ok i hear you @eliotwrobson !

so let me make a small list of todos' for this issue here - Eliot - can you fill in any gaps for me please? i haven't been able to dig deeply into this review but do want to help it move forward

  • there is one potentially outstanding request from @phildong to add "tutorials" or quick start examples for using automata. btw i really appreciate the issue that you opened here!! BUT with that said i see this here . This is what we might consider a quickstart or "vignette" in the r world! so you do have that!

I also see that for someone who doesn't know what automata is, it would be hard to follow that vignette and actually understand what it was doing. I wonder if we could discuss this in our slack with the community - the question here is about how rigorous docs should be when we accept vs if there is something that is good enough for now - and we know the authors are actively working on this. What do you all think?

  • @eliotwrobson have you considered updating your github repo link to be your documentation rather than pypi so it's easier to find for people? At first i thought you didn't have docs at all but you do - and they are nice! (mkdocs is so clean!).
  • we are missing a second review submitted to this issue from @irisdyoung .

i think we can discuss the first check above together. either here / on more synchronously in our slack. let me know what you all prefer. in the meantime i am going to pose the question in slack as it's really a standards related question for our review.

@eliotwrobson
Copy link
Author

eliotwrobson commented Jun 7, 2024

@lwasser thanks! discussing on slack is probably easier, I'm not on there currently but you can send me an invite at eliot.robson24@gmail.com!

Regarding the docs link, that seems like a good change, so I'll make a PR for that as soon as I get the chance 👍

Edit: actually, I think the repo link on pypi should stay as-is? The docs link there is correct, and the repo includes a docs link as well.

Edit 2: ahh I see what you mean now. Will discuss with the other maintainer.

Edit 3: Github repo link has been updated!

@NimaSarajpoor
Copy link

NimaSarajpoor commented Jun 18, 2024

@eliotwrobson
I noticed an issue was already opened to enhance the examples. I checked the changes made to docs/examples/fa-examples.md. Great improvement! I am going to provide my comments just to make sure we have all things in one place. Btw, your package might be useful for engineers as well as they can better understand the concept of state / transition.

Please feel free to ignore if a similar comment was made by someone else before, and was already addressed / discussed. Also, these are just my perspective. So, feel free to let me know when you think a comment is not a good fit.

(1) Maybe add a link to examples on README page (Or maybe just provide one example on the README page)
(2) If interested in getting more citations, you may want to add a citing part on README
(3) Adding diagram (which I can see that it is addressed by calling the method show_diagram() in this opened issue)
(4) Adding (printing) the output of read_user_input(my_dfa) can help user connect the dots (particularly, if they are trying to learn this concept, seeing the output may give them the "Eureka moment!")
(5) Add the description of non-private methods to the doctoring of class DFA (I haven't checked other classes)

I do not see any other concerns from my end!

@eliotwrobson
Copy link
Author

@NimaSarajpoor Thank you for the feedback! I think we should be able to add most of those items on to the open pull request you linked to.

The only one I had questions on was 5. Would this just be listing every public method in the top docstring for the DFA class? I'm not sure how valuable this would be in the scheme of things, since there's a huge number of methods, and these are all listed on the documentation site anyway. However, I'm not sure what common practice is in other libraries.

@lwasser
Copy link
Member

lwasser commented Jun 22, 2024

Hi there - i'm just checking in to see how things are going 👋🏻 it looks like there are a few wrapup documentation elements to complete and then we can move this forward. @NimaSarajpoor THANK YOU for pitching in here!! i'll also add you as a reviewer to the header.

finally i am curious about the description being requested - i do see this

https://caleb531.github.io/automata/api/fa/class-dfa/

in the api docs and it looks like methods are generally documented. We can discuss more however as there might be a documentation piece that i'm missing as well!

Have a great weekend y'all. i'll check back in next week and we can work together to wrap this review up!

@NimaSarajpoor
Copy link

TL;DR
Probably better to ignore (5)


@lwasser

finally i am curious about the description being requested - i do see this

You are absolutely right that the description was provided in the API docs. My comment was mainly about having the methods in the doctoring of the class

https://github.com/caleb531/automata/blob/530016a03703e071b72f1665769770039018536d/automata/fa/dfa.py#L54

@eliotwrobson

The only one I had questions on was 5. Would this just be listing every public method in the top docstring for the DFA class?

However, I'm not sure what common practice is in other libraries.

After you ask the question, I realized I was not sure myself! 😅 I did some research and found this stackoverflow post in which the answer quotes the following sentence from PEP 257:

The docstring for a class should summarize its behaviour and list the public methods and instance variables.

Still, the author of the answer mentioned that they prefer to avoid doing this because it can become out of date in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: under-review
Development

No branches or pull requests

7 participants