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

compute maximum bisimulation using Paige-Tarjan's Algorithm #1089

Merged
merged 32 commits into from
Jun 22, 2024

Conversation

sander-hergarten
Copy link
Contributor

regarding #1075

@CLAassistant
Copy link

CLAassistant commented Feb 15, 2024

CLA assistant check
All committers have signed the CLA.

@sander-hergarten
Copy link
Contributor Author

i had some trouble running tox -edocs on my machine. Please let me know if there are any issues with the documentation or release notes

Copy link
Collaborator

@IvanIsCoding IvanIsCoding left a comment

Choose a reason for hiding this comment

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

I am going to read more about the algorithm before doing the proper review.

I left some comments about Rust/Python but overall it is in good shape! And the tox -edocs failure is common, identation is probably mismatched but it should be easy to fix

tests/digraph/test_bisimulation.py Outdated Show resolved Hide resolved
src/iterators.rs Outdated
Comment on lines 909 to 914
impl PyHash for NodeIndices {
fn hash<H: Hasher>(&self, py: Python, state: &mut H) -> PyResult<()> {
PyHash::hash(&self.nodes, py, state)?;
Ok(())
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

If this is not requried I'd rather not make this type hashable. We reserve the right to make it mutable one day which would make it not hashable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I implemented this for the RelationalCoarsestPartition return type, since custom_vec_iter_impl! requires this trait. If you would prefer, I can make a new iterator that will act as a container.

Since RelationalCoarsestPartition is also a pretty specific type name i could change it to something like NodePartition containing something such as NodePartitionBlock or such.

Copy link
Collaborator

Choose a reason for hiding this comment

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

In this case I would implement a new type and add a macro custom_vec_iter_impl! calling the old custom_vec_iter! and adding the PyHash. The catch is that sometimes these methods can have unintended side effects on performance (e.g. #1096) so it is better to be conservative

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure I understood this but the hashing from NodeIndices was removed

@coveralls
Copy link

coveralls commented Feb 15, 2024

Pull Request Test Coverage Report for Build 9621638477

Details

  • 297 of 309 (96.12%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.005%) to 95.868%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/bisimulation.rs 294 296 99.32%
src/iterators.rs 0 10 0.0%
Totals Coverage Status
Change from base Build 9620550342: 0.005%
Covered Lines: 17886
Relevant Lines: 18657

💛 - Coveralls

@sander-hergarten
Copy link
Contributor Author

As a heads up, when reading "Three Partition Refinement Algorithms", I renamed a lot of the variables.

  • Blocks of Partition $X$: CoarseBlock
  • Blocks of Partition $Q$: FineBlock
  • $C$: queue
  • $S$: splitter_block
    Also while the authors say that blocks of $Q$ are contained in blocks of $X$ I interpreted this as the block of $Q$ being a subset of $X$. (I found this confusing so i thought i would mention it)

Copy link
Collaborator

@IvanIsCoding IvanIsCoding left a comment

Choose a reason for hiding this comment

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

Adding some Rust comments this time

src/bisimulation.rs Outdated Show resolved Hide resolved
src/bisimulation.rs Outdated Show resolved Hide resolved
src/bisimulation.rs Outdated Show resolved Hide resolved
@mtreinish mtreinish added this to the 0.15.0 milestone Feb 21, 2024
Copy link
Collaborator

@IvanIsCoding IvanIsCoding left a comment

Choose a reason for hiding this comment

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

I think the algorithm looks good after skimming the original paper from 1987 and the BisPy implementation. I will also trust the tests and the code coverage essentially touching all lines.

Given how much data is shared I am also don't mind all Rc and RefCell. In fact, it is quite impressive that this is correct and Rust's borrow checker approves. Quite an achievement for a first time contribution!

I think after we address the hashing it should be good to merge. Notice that #1096 will introduce a compile error with your change because of the new optimization, but that should be easyd to fix.

src/bisimulation.rs Show resolved Hide resolved
@IvanIsCoding IvanIsCoding added the automerge Queue a approved PR for merging label Jun 22, 2024
@IvanIsCoding IvanIsCoding merged commit be22da1 into Qiskit:main Jun 22, 2024
28 of 30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Queue a approved PR for merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants