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

Fix BasisEmbedding to allow only-1 bitstrings #1114

Merged
merged 15 commits into from
Mar 2, 2021
Merged

Fix BasisEmbedding to allow only-1 bitstrings #1114

merged 15 commits into from
Mar 2, 2021

Conversation

mariaschuld
Copy link
Contributor

Fixes issue #1112.

@github-actions
Copy link
Contributor

Hello. You may have forgotten to update the changelog!
Please edit .github/CHANGELOG.md with:

  • A one-to-two sentence description of the change. You may include a small working example for new features.
  • A link back to this PR.
  • Your name (or GitHub username) in the contributors section.

.github/CHANGELOG.md Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Feb 26, 2021

Codecov Report

Merging #1114 (d2a5ff4) into master (94cf6d4) will increase coverage by 2.79%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1114      +/-   ##
==========================================
+ Coverage   94.94%   97.74%   +2.79%     
==========================================
  Files         155      155              
  Lines       11753    11753              
==========================================
+ Hits        11159    11488     +329     
+ Misses        594      265     -329     
Impacted Files Coverage Δ
pennylane/templates/embeddings/basis.py 100.00% <100.00%> (ø)
pennylane/math/tensorbox.py 95.97% <0.00%> (+1.34%) ⬆️
pennylane/beta/devices/default_tensor.py 95.23% <0.00%> (+1.70%) ⬆️
pennylane/interfaces/tf.py 88.75% <0.00%> (+2.50%) ⬆️
pennylane/devices/default_qubit_tf.py 90.66% <0.00%> (+2.66%) ⬆️
pennylane/math/numpy_box.py 98.55% <0.00%> (+2.89%) ⬆️
pennylane/beta/devices/default_tensor_tf.py 87.50% <0.00%> (+3.12%) ⬆️
pennylane/tape/qnode.py 98.93% <0.00%> (+3.91%) ⬆️
pennylane/interfaces/torch.py 93.06% <0.00%> (+3.96%) ⬆️
pennylane/tape/interfaces/torch.py 100.00% <0.00%> (+5.26%) ⬆️
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 94cf6d4...d2a5ff4. Read the comment docs.

Copy link
Member

@josh146 josh146 left a comment

Choose a reason for hiding this comment

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

Thanks @mariaschuld for getting to this so quickly!

Even though this is very trivial, should we add tests for the all zeros and all ones case, just to make sure it is correctly validated? This will also help catch similar bugs in the future.

.github/CHANGELOG.md Outdated Show resolved Hide resolved
pennylane/templates/embeddings/basis.py Outdated Show resolved Hide resolved
@josh146 josh146 added the bug 🐛 Something isn't working label Feb 28, 2021
@mariaschuld
Copy link
Contributor Author

Good idea, added new test cases...

@josh146 josh146 merged commit 8031ba9 into master Mar 2, 2021
@josh146 josh146 deleted the fix1112 branch March 2, 2021 14:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The BasisEmbedding template does not allow for all-ones or all-zero basis states
3 participants