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

Fwdsim patches #517

Merged
merged 3 commits into from
Jan 13, 2025
Merged

Fwdsim patches #517

merged 3 commits into from
Jan 13, 2025

Conversation

coreyostrove
Copy link
Contributor

@coreyostrove coreyostrove commented Dec 17, 2024

This PR is a set of patches related to #496, which was just merged in.

Substantive changes:

  • The casting behavior of ForwardSimulator has been updated so that the 'auto' keyword argument now defaults to the MapForwardSimulator in all cases (previously this was only true for qudit models or for two-or-more qubits).
  • The germ selection algorithm now automatically converts to a MatrixForwardSimulator if needed.

Unit Test Fixes

  • Some unit tests on beta which started failing after Making 2Q GST Go Vroom Vroom #496 flagged the fact that there were a few tests in test_packages which were written assuming the default simulator was MatrixForwardSimulator. A whole bunch more broke for this reasone when I made the patch above changing the default casting behavior. Where appropriate I've updated these tests to be compatible with both simulators. In places where the MatrixForwardSimulator really was required (e.g. dproduct and hproduct methods were required for the thing being tested) I've made sure those tests have had the proper simulator specified explicitly.
  • Spring cleaning: While doing these fixes I bumped into some tests which were making use of an old Model method which no longer exists. I went ahead and deleted these for the sake of cleaning things up. These tests were already being skipped or commented out, so there is no change to the tests being run.

Corey Ostrove added 3 commits December 16, 2024 20:41
Changes the default behavior of casting with the 'auto' keyword to use the map forward simulator.

Update germ selection to automatically convert to a matrix forward simulator as needed.
Fix tests that were broken by the switch to using map as the default forward simulator. Either update tests to support map natively, or for ones that rely on matrix make sure that is the simulator being used.
Remove a few unit tests that made reference or relied on an old model method that no longer exists. These tests were already being skipped (or had the sections referencing this old method commented out), so this doesn't actually affect anything other than making stuff cleaner.
Copy link
Contributor

@rileyjmurray rileyjmurray left a comment

Choose a reason for hiding this comment

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

Looks good to me!

Copy link
Contributor

@sserita sserita left a comment

Choose a reason for hiding this comment

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

All looks good, thanks @coreyostrove!

@sserita sserita merged commit fb997d4 into develop Jan 13, 2025
4 checks passed
@sserita sserita deleted the fwdsim-patches branch January 13, 2025 21:36
@sserita sserita added this to the 0.9.13 milestone Jan 13, 2025
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.

3 participants