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

Improve Documentation Site #63

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

IsaccBarker
Copy link
Collaborator

Fix #60.

@IsaccBarker IsaccBarker self-assigned this Nov 13, 2024
@IsaccBarker IsaccBarker marked this pull request as draft November 13, 2024 19:13
@IsaccBarker
Copy link
Collaborator Author

Implemented; facing some issues where items aren't included in the right toctree, will submit a PR when fixed.

@gvegayon gvegayon marked this pull request as ready for review November 22, 2024 20:53
@IsaccBarker
Copy link
Collaborator Author

image
image

Can we check this?

@IsaccBarker
Copy link
Collaborator Author

We're seeing a new Resource not accessible by integration error coming from GraphQL via the action that adds comments to the PR chain. Did you change some security settings?

@IsaccBarker
Copy link
Collaborator Author

@gvegayon can you remove Markdown checks for Codacy? It's giving a bunch of false positives (e.g. multiple top-level headings despite that being visually optimal and what Quarto outputs).

@IsaccBarker
Copy link
Collaborator Author

See #70 for an explanation on why Python 3.7 is failing.

@IsaccBarker
Copy link
Collaborator Author

We're seeing a new Resource not accessible by integration error coming from GraphQL via the action that adds comments to the PR chain. Did you change some security settings?

Since this isn't working, you can download the artifact here.

@gvegayon gvegayon requested a review from apulsipher January 8, 2025 22:55
Copy link
Contributor

@apulsipher apulsipher left a comment

Choose a reason for hiding this comment

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

A couple of questions and fixes


# Installation

- clone this repository
- `pip install ./epiworldpy`
Installation can be preformed through pip (pip installs packages).
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo. "preformed" should be "performed". Might be clearer to simply rewrite this line as "Install epiworld with pip:"

Copy link
Member

Choose a reason for hiding this comment

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

Make an inline suggestion!

> dynamics. Furthermore, epiworldR is ideal for simulation studies featuring large
> populations.

Current available models:
Copy link
Contributor

Choose a reason for hiding this comment

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

Are all of these available in the python version? Because any models that aren't should be somehow marked as such (indicate only available in C++ but will come to python eventually)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch. All of these are implemented, except SIRLogit and SIRMixing. Expect a PR later today with these implemented, or with them removed from the list here.

Computing some key statistics.

```{python}
# ...
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this section supposed to be blank?

Copy link
Collaborator Author

@IsaccBarker IsaccBarker Jan 14, 2025

Choose a reason for hiding this comment

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

Unsure why that's blank, Computing some key statistics shouldn't actually even be there as it doesn't match up with the R site. Will remove.


# If extensions (or modules to document with autodoc) are in another directory,
# add these directories to sys.path here. If the directory is relative to the
# documentation root, use os.path.abspath to make it absolute, like shown here.
#sys.path.insert(0, os.path.abspath('.'))
#sys.path.insert(0, os.path.abspath('../src/epiworldpy'))
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not familiar with Sphinx, so for my own edification, why are so many lines commented like this in this file?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the default Sphinx configuration file that ships with sphinx-quickstart, which I assume is what George used to create the documentation in the first place. The commented out configuration lines are the default behaviour (one other such example of this might be sshd_config(5)). That line is supposed to add a directory to the Python module search path, so the documentation can be generated for HEAD instead of what's installed locally on the system.

I think while setting up the API documentation I un-commented this line (already present in the quickstart), made a change, then realized it didn't work and re-commented it. Looking at the template the quick start uses confirms this theory (sys.path.insert(0, {{ module_path | repr }})). The value in the comment is more accurate to our project structure now, but it may warrant removal due to it not doing anything. Thoughts?

@apulsipher
Copy link
Contributor

See #70 for an explanation on why Python 3.7 is failing.

@IsaccBarker I'm not seeing the explanation for Python failing in #70

@IsaccBarker
Copy link
Collaborator Author

See #70 for an explanation on why Python 3.7 is failing.

@IsaccBarker I'm not seeing the explanation for Python failing in #70

Sorry for the delay, I should have mentioned #71. Off by one, my apologies.

@apulsipher
Copy link
Contributor

We're seeing a new Resource not accessible by integration error coming from GraphQL via the action that adds comments to the PR chain. Did you change some security settings?

Since this isn't working, you can download the artifact here.

@IsaccBarker Can you create an issue to fix this?

@IsaccBarker IsaccBarker added changes requested Changes were requested before merging. blocked Blocked by another issue and removed changes requested Changes were requested before merging. labels Jan 14, 2025
@IsaccBarker
Copy link
Collaborator Author

Blocked until CI actions are updated on main.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked Blocked by another issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve Documentation Site
3 participants