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

parallel_embeddings.py and feasibility.py minor corrections #259

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

Conversation

jackraymond
Copy link
Contributor

Minor improvements to new parallel_embeddings.py and feasibility.py modules:

  • max_num_emb should be optional, allowing a None value that indicates no bound on the number of embeddings. This has been implemented.
  • The lattice_size() function is merely a helper function for find_sublattice_embeddings() and doesn't have much generalization power beyond this. For this reason I have moved it into the parallel_embeddings.py module. Separately, a new non-trivial upper-bound function could be added to feasibility.py as a feature enhancement (noted in comments).

Andy Zhang and others added 3 commits December 23, 2024 11:05
jackraymond added a commit to AndyZzzZzzZzz/shimming-tutorial that referenced this pull request Dec 27, 2024
@@ -128,7 +128,7 @@ def find_multiple_embeddings(
S: The source graph to embed.
T: The target graph in which to embed.
max_num_emb: Maximum number of embeddings to find.
Defaults to 1, set to float('Inf') to find the maximum possible
Defaults to 1, set to None to find the maximum possible
Copy link
Member

Choose a reason for hiding this comment

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

This is actually a breaking change. For backwards compatibility, consider supporting inf as well, but throwing a deprecation warning if called with it. (I haven't checked, but if Inf support hasn't been released yet, than no need to deprecate it, obviously.)

Copy link
Member

Choose a reason for hiding this comment

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

Indeed, this code hasn't been released yet, so please ignore the comment above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The first pull request was merged prematurely. I think @thisac wants to take a second look at this tidy up request as well.

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