Skip to content

Add Python CI, update API docs, and fix Rust append behavior#81

Open
mathleur wants to merge 2 commits intofeature/rustfrom
feat/improvements
Open

Add Python CI, update API docs, and fix Rust append behavior#81
mathleur wants to merge 2 commits intofeature/rustfrom
feat/improvements

Conversation

@mathleur
Copy link
Member

Description

Contributor Declaration

By opening this pull request, I affirm the following:

  • All authors agree to the Contributor License Agreement.
  • The code follows the project's coding standards.
  • I have performed self-review and added comments where needed.
  • I have added or updated tests to verify that my changes are effective and functional.
  • I have run all existing tests and confirmed they pass.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR updates CI and documentation while addressing a Rust Qube::append edge case when appending into an empty receiver.

Changes:

  • Add an empty-receiver fast-path to Qube::append and a regression test covering append-to-empty behavior.
  • Expand Rust and Python API documentation to include additional manipulation methods (drop, squeeze, etc.).
  • Update GitHub Actions CI to run Python binding tests inside an isolated uv-managed virtual environment.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
qubed/src/merge.rs Adds an is_empty() fast-path for append by copying other into self and clearing other.
qubed/tests/test_union.rs Adds a test for appending into an empty Qube; removes commented-out code.
docs/src/rust/qubed.md Documents additional Rust API methods and adds examples for drop/squeeze.
docs/src/python/py_qubed.md Adds Python docs for compress, drop, squeeze, and query methods.
.github/workflows/ci.yml Runs Python tests in a dedicated uv venv rather than the global runner environment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +124 to +131
let expected_ascii = other.to_ascii();

empty.append(&mut other);

assert_eq!(
empty.to_ascii(),
expected_ascii,
"appending to an empty Qube should yield the other Qube's content"
Copy link

Copilot AI Mar 20, 2026

Choose a reason for hiding this comment

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

This test locks in append producing exactly the pre-append other.to_ascii(). If append is meant to always call compress() (as described in docs / append implementation comments), the post-append ASCII can legitimately differ while remaining semantically equivalent. Consider asserting semantic equivalence (e.g., via to_datacubes()/datacube_count()), or update the expected ASCII to match the compressed form if compression is intended.

Suggested change
let expected_ascii = other.to_ascii();
empty.append(&mut other);
assert_eq!(
empty.to_ascii(),
expected_ascii,
"appending to an empty Qube should yield the other Qube's content"
let expected_hash = other
.node(other.root())
.unwrap()
.structural_hash();
empty.append(&mut other);
let empty_hash = empty
.node(empty.root())
.unwrap()
.structural_hash();
assert_eq!(
empty_hash,
expected_hash,
"appending to an empty Qube should yield the other Qube's content (semantically)"

Copilot uses AI. Check for mistakes.
| `get_or_create_child` | `fn get_or_create_child(&mut self, key: &str, parent_id: NodeIdx, coordinates: Option<Coordinates>) -> Result<NodeIdx, String>` | Return the existing child with the given dimension+coordinates, or create a new one. |
| `check_if_new_child` | `fn check_if_new_child(&mut self, key: &str, parent_id: NodeIdx, coordinates: Option<Coordinates>) -> Result<bool, String>` | Return `true` if no child with the given dimension+coordinates exists yet. |
| `remove_node` | `fn remove_node(&mut self, id: NodeIdx) -> Result<(), String>` | Remove a node and all its descendants |
| `append` | `fn append(&mut self, other: &mut Qube)` | Union: merge `other` into `self`, compress, then clear `other` |
Copy link

Copilot AI Mar 20, 2026

Choose a reason for hiding this comment

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

The method table describes append as “merge other into self, compress, then clear other”. With the new empty-receiver fast-path in Qube::append, compress() is no longer guaranteed to run. Either keep append semantics consistent by compressing even in the fast-path, or update this documentation to describe the conditional behavior.

Suggested change
| `append` | `fn append(&mut self, other: &mut Qube)` | Union: merge `other` into `self`, compress, then clear `other` |
| `append` | `fn append(&mut self, other: &mut Qube)` | Union: merge `other` into `self`, then clear `other`. If `self` was non-empty, run `compress()` afterward. |

Copilot uses AI. Check for mistakes.

#### `compress() -> None`

Compress the Qube in-place. Merges structurally identical sibling nodes, removes empty nodes, and deduplicates. Called automatically by `append` and `append_many`.
Copy link

Copilot AI Mar 20, 2026

Choose a reason for hiding this comment

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

This section states compress() is called automatically by append/append_many, but Qube::append now returns early when the receiver is empty without calling compress(). If that behavior is intentional, the Python docs should mention the exception; otherwise, consider ensuring append always compresses so the docs remain accurate.

Suggested change
Compress the Qube in-place. Merges structurally identical sibling nodes, removes empty nodes, and deduplicates. Called automatically by `append` and `append_many`.
Compress the Qube in-place. Merges structurally identical sibling nodes, removes empty nodes, and deduplicates. Typically called automatically by `append` and `append_many`, except when appending to an empty `PyQube` where `append` may return early without triggering compression. Call `compress()` explicitly if you need to ensure a canonical form.

Copilot uses AI. Check for mistakes.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants