-
Notifications
You must be signed in to change notification settings - Fork 45
Add OperatorStyle and GeometryStyle
#352
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 4 files with indirect coverage changes 🚀 New features to boost your workflow:
|
src/utility/styles.jl
Outdated
| IsfiniteStyle(::T) where {T<:YourType} | ||
| ``` | ||
| """ | ||
| abstract type IsfiniteStyle end |
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.
Same comment here
|
Except for the comments, looks good to me, I'm happy with this and think it will overall improve the quality of the package. |
OperatorStyle and IsfiniteStyleOperatorStyle and GeometryStyle
|
I fully agree with both the comments and the renaming and have changed the PR accordingly. |
|
One question I came up with when rewriting the environment constructors is: In order to not always have to write e.g. @assert allequal([GeometryStyle(above),GeometryStyle(below),GeometryStyle(operator)])I would argue for overloading e.g. Base.:&(::FiniteStyle, ::FiniteStyle) = FiniteStyle()
Base.:&(::InfiniteStyle, ::InfiniteStyle) = InfiniteStyle()Then, one can call geometry_style = GeometryStyle(above) & GeometryStyle(below) & GeometryStyle(operator)which will either give the common style of the 3 or error, if they are not identical. (If we want to throw a specific kind of error, one can also overload Base.:&(::FiniteStyle, ::InfiniteStyle) = throw(...)
Base.:&(::InfiniteStyle, ::FiniteStyle) = throw(...) |
|
Your PR no longer requires formatting changes. Thank you for your contribution! |
|
Just looked through the PR and talked to @lkdvos. I like this idea and support it being added to the code, provided that someone is appointed to actually do all the other changes to accomodate this idea :) . |
|
I am currently already working on implementing the changes. |
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.
I've pushed some final changes to the formatting and docstrings, to address the comment about MPO style referring to transfer matrix.
Once the tests turn green, I will merge this.
This PR is the start of some work to make it possible/easier to add custom state or operator types without having to alter their base implementations, and without having to redefine a bunch of functionality that already just works.
The main idea is to avoid having to encode distinctions between finite/infinite or hamiltonian/mpo as hard type constants or abstract types, since this often clashes with Julia's type tree architecture (you cannot subtype multiple things).
Instead the idea is to use traits for dispatching, and gradually start loosening the type restrictions on the inputs of various functions here.
The plan is to gradually start incorporating this strategy, to avoid having a massive PR that has to change everything at once, and keep this non-breaking.
This PR is the first part, which simply introduces the traits.