-
Notifications
You must be signed in to change notification settings - Fork 25
Add single-layer-pepo reduced densitymatrix #254
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
Conversation
Codecov Report❌ Patch coverage is
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
|
I've added tests on |
lkdvos
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for putting that together and fixing this. I don't have too much time this week, so if you want to change the error messages to be more informative and merge this, I'm definitely happy with that.
Yue-Zhengyuan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have made some minor changes to the docstring and error messages. The format check error seems to be an issue with GitHub. OK to merge for me now.
|
Should resolve itself in a bit, I'll try and retrigger the workflows this afternoon. |
I just found out there are issues with PEPOs with a non-trivial unit cell. I'll try to fix it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed physicalspace for PEPO to return the physical codomain space in the bottom layer directly as a Nrow x Ncol matrix (no 3rd dimension). The original implementation returns something strange (e.g. for an 2 x 2 x 1 PEPO, it returns a length-2 vector). Things should be fine now. @lkdvos You may want to take a final look before merging.
No description provided.