Add a tree property to the Tree class #39
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bsweger
force-pushed
the
bsweger/add-tree-class/28
branch
4 times, most recently
from
57ed74f to
46edae9
Compare
bsweger
commented
Oct 23, 2024
README.md
Outdated
|
|
||
| ## Docker Setup |
There was a problem hiding this comment.
Opened an issue to move the setup instructions to a new CONTRIBUTING.md file.
At this point, the dev setup distracts from the main focus of the page, which is how to get started.
Apologies for the odd diff!
bsweger
commented
Oct 23, 2024
docs/user-guide.rst
Outdated
There was a problem hiding this comment.
Made some heavy edits to better split up the examples.
bsweger
commented
Oct 23, 2024
| @@ -150,7 +149,7 @@ def sequence_metadata(self): | |||
| @sequence_metadata.getter | |||
| def sequence_metadata(self) -> pl.LazyFrame: | |||
| """ | |||
| :class:`polars.LazyFrame` : A Polars LazyFrame that references | |||
| :external+polars:std:doc:`polars.LazyFrame<reference/lazyframe/index>` : A Polars LazyFrame that references | |||
There was a problem hiding this comment.
Finally figured out how to link to documentation for other projects that use sphinx
bsweger
commented
Oct 23, 2024
| @@ -25,7 +25,7 @@ def _get_session(retry: bool = True) -> requests.Session: | |||
| total=5, | |||
| allowed_methods=frozenset(["GET", "POST"]), | |||
| backoff_factor=1, | |||
| status_forcelist=[401, 403, 404, 429, 500, 502, 503, 504], | |||
| status_forcelist=[429, 500, 502, 503, 504], | |||
There was a problem hiding this comment.
There's no reason to retry 401, 403, and 404 status codes since we don't expect the result to change.
bsweger
force-pushed
the
bsweger/add-tree-class/28
branch
3 times, most recently
from
1d17926 to
623a70d
Compare
bsweger
added a commit
that referenced
this pull request
Oct 29, 2024
Docker has been working nicely for our project setup and GitHub actions, but this changeset removes it. After a bit of thrashing about the best way to invoke the Nextclade CLI from this package, we landed on using their Docker image (#39) To avoid getting into the business of invoke Docker commands within a Docker container, it will be cleaner to avoid recommended its use for dev purposes. The plan in a future PR is to move the project setup instructions from the main README to a CONTRIBUTING.md and point people to uv for local setup (which will hopefully address some of our original reasons for dockerizing the project, e.g., preventing the need to install Python)
bsweger
added a commit
that referenced
this pull request
Oct 29, 2024
Docker has been working nicely for our project setup and GitHub actions, but this changeset removes it. After a bit of thrashing about the best way to invoke the Nextclade CLI from this package, we landed on using their Docker image (#39) To avoid getting into the business of invoke Docker commands within a Docker container, it will be cleaner to avoid recommended its use for dev purposes. The plan in a future PR is to move the project setup instructions from the main README to a CONTRIBUTING.md and point people to uv for local setup (which will hopefully address some of our original reasons for dockerizing the project, e.g., preventing the need to install Python)
bsweger
added a commit
that referenced
this pull request
Oct 30, 2024
Docker has been working nicely for our project setup and GitHub actions, but this changeset removes it. After a bit of thrashing about the best way to invoke the Nextclade CLI from this package, we landed on using their Docker image (#39) To avoid getting into the business of invoke Docker commands within a Docker container, it will be cleaner to avoid recommended its use for dev purposes. The plan in a future PR is to move the project setup instructions from the main README to a CONTRIBUTING.md and point people to uv for local setup (which will hopefully address some of our original reasons for dockerizing the project, e.g., preventing the need to install Python) This is a temporary step, because uv is looking for uv.lock Will fix this up when addressing #40
The nextclade_dataset_name returned in https://nextstrain-data.s3.amazonaws.com/files/ncov/open/metadata_version.json is 'SARS-CoV-2', which is a shortcut for nextstrain/sars-cov-2/wuhan-hu-1/orfs. This changeset adds the full name to the metadata, so we can use it when retrieving versioned reference trees.
First pass at a new tree class. For now, the only thing it does is generate a URL to the nextclade SARS-CoV-2 reference tree that was effective on the "tree as_of date"
It was confusing and wasn't working. The operations of the Tree class small, and it will be more straightforward to test them via the integration suite.
Also move the tree tests to the integration suite, since we're doing network connections.
This changeset explores the world of intersphinx mapping. It adds a reference to the nextstrain docs (which are also sphinx-based) in case we need it, and it adds an external polars reference to a CladeTime docstring
This also fixes up some formatting in the existing docs.
It's unclear if there's any utility in being able to link to an "as_of" reference tree and browse it online, so let's keep the url property for now, but caution that it's experimental (since Nextstrain can't guarantee that their URL structure won't change)
This changeset uses ncov metadata to get the nextclade dataset that corresponds to tree_as_of and extract tree.json from it. Because this pass uses "docker run" to invoke the nextclade CLI, our Docker-based GitHub actions will fail, since we can't invoke docker within docker. Next commit will address this.
bsweger
force-pushed
the
bsweger/add-tree-class/28
branch
from
2fd6e97 to
2a163c7
Compare
elray1
approved these changes
Oct 31, 2024
bsweger
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves #28
This PR adds a
Treeclass to cladetime.Treehas two properties: url to a reference tree, and the tree itself.The changes also include improvements to the existing documentation, as well as new documentation for
Tree.Note to reviewer(s)
A great way to test the new features, would be to give the Tree documentation example a spin.