Skip to content

Conversation

michaelosthege
Copy link
Member

I went over the files under /docs/source/contributing/* except the ones that had an :orphan: line at the top.

I wrote the instructions on running the test suite and fixed typos in several places.

For the guide on implementing distributions I updated it to the best of my knowledge, but here @ricardoV94 should fact-check me.

Closes #5346


Some remarks that generalize to other pages:

  • I changed the formatting of "Project" and "packages" to be consistent w.r.t. capitalization and style:
    • NumPy or numpy
    • SciPy or scipy
    • PyMC or pymc
    • Aesara or aesara
  • Instead of "PyMC 4.x" we should write "PyMC >=4.0.0" so we don't have to update these lines with the next major release.
  • cc @OriolAbril I would generally prefer a "one line per sentence" rule in the docs sources, because it makes the git diff a lot easier to work with. (Only double line-breaks become line breaks in the rendered output.)

@michaelosthege michaelosthege added this to the v4.0.0 milestone May 26, 2022
@OriolAbril
Copy link
Member

cc @OriolAbril I would generally prefer a "one line per sentence" rule in the docs sources, because it makes the git diff a lot easier to work with. (Only double line-breaks become line breaks in the rendered output.)

That is perfectly fine with me, I also prefer this style generally

except the ones that had an :orphan: line at the top.

Closes #5346

To close the issue all pages should be updated, independently of having :orphan: or not. afaik, all pages are currently present in one of the multiple toctrees and show in the left sidebar, the orphan tags are inherited and left in the pages that still have not been touched yet. Most relevant on this and to close this issue is https://docs.pymc.io/en/latest/contributing/developer_guide.html.

Copy link
Member

@ricardoV94 ricardoV94 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The developer distribution part looks good, just an issue with mentioning aeppl

@michaelosthege
Copy link
Member Author

@ricardoV94 I updated the beginning of the developer_guide.rst to something that's hopefully correct and more accessible to people without a strong math background (please check).

I stopped where it got more detailed on logp. Can you take over from there?

Large parts of that document are written in the "I" perspective and talk about things that are no longer relevant.

Generally, I would prefer to delete large sections instead of keeping things that are outdated or misleading.

We could also move some submodule-specific developer documentation such as the "Dynamic HMC" or "Variational Inference" sections into README.md files that are located in the corresponding subfolders.
Maybe that would help to keep them aligned with the implementation while keeping the docs focused on user API ?

Copy link
Member

@OriolAbril OriolAbril left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also move some submodule-specific developer documentation such as the "Dynamic HMC" or "Variational Inference" sections into README.md files that are located in the corresponding subfolders.
Maybe that would help to keep them aligned with the implementation while keeping the docs focused on user API ?

I always advocate for having documentation in the docs. Pages outside the "contributing" navbar section have users as target audience, pages inside this section have developers and contributors as target audience. IMO, having the docs close or even in the same file has a negligible effect on keeping them updated when compared to making sure this is done as part of the review process. i.e. even docstrings that are as close as possible to the implementation still end up outdated quite often

cc @danhphan I think this should fix some of the issues you mentioned on the running tests page. Is there something else you think is missing?

@@ -1,6 +1,6 @@
# Implementing a Distribution
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the import paths need to be fixed here in multiple places. i.e. pymc.Distribution instead of pymc.distributions.Distribution

@michaelosthege
Copy link
Member Author

I formatted the dev guide as a Markdown file to make it easier to update.

I only dealt with the beginning, but I wont have the time to continue with the rest.
Therefore I would recommend to (squash) merge the changes I made so far and continue updating the dev guide in the coming days.

@danhphan
Copy link
Member

Hi @OriolAbril I think it looks good now. Thanks mate :)

@twiecki
Copy link
Member

twiecki commented Jun 3, 2022

Can we merge this?

@ricardoV94
Copy link
Member

The changes to the dev guide look good. We will probably want to merge this with the new document that is being added in #5721 but for now we can keep both.

@ricardoV94
Copy link
Member

ricardoV94 commented Jun 3, 2022

@twiecki
Copy link
Member

twiecki commented Jun 3, 2022

Why is this not a NB?

@twiecki
Copy link
Member

twiecki commented Jun 3, 2022

Maybe we'll remove it for now?

@ricardoV94
Copy link
Member

Maybe we'll remove it for now?

Let's keep the file around but not show it in the docs? Is this possible @OriolAbril
I'll branch of this PR with the remaining commits, as those seem still useful.

@ricardoV94 ricardoV94 modified the milestones: v4.0.0, v4.1.0 Jun 3, 2022
@OriolAbril
Copy link
Member

Let's keep the file around but not show it in the docs? Is this possible @OriolAbril

Yes, it is possible, we just need to add

---
orphan: true
---

at the top of the file and remove it from the toctree in https://github.com/pymc-devs/pymc/blob/main/docs/source/contributing/index.md which seems to have happened already

@michaelosthege
Copy link
Member Author

@OriolAbril can you fix the precommit here?

The passage is written to literally have the URLs in there, but the pre-commit doesn't allow it.

@OriolAbril
Copy link
Member

@OriolAbril can you fix the precommit here?

The passage is written to literally have the URLs in there, but the pre-commit doesn't allow it.

Updated the doc (that I copied over unmodified not long ago while fixing the website and navbar) and added the file to the pre-commit exclusion for the cross-reference task. Here the urls must be present, so it needs to be excluded from that check

@michaelosthege
Copy link
Member Author

michaelosthege commented Jun 11, 2022

/pre-commit-run

@michaelosthege
Copy link
Member Author

Ok, so the page is now orphaned, and git conflicts were resolved.

Can we merge now? Because this PR adds new content on contributing pages that were previously filled with ToDo placeholders.

@michaelosthege michaelosthege merged commit 3f2afb2 into main Jun 11, 2022
@michaelosthege michaelosthege deleted the update-dev-guide branch June 11, 2022 17:59
@michaelosthege michaelosthege modified the milestones: v4.1.0, v4.0.1 Jul 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Developer guide needs to be updated for V4

5 participants