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
Dimensions class #1996
Dimensions class #1996
Conversation
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 done an initial review of the Qobj part and the basic metaclasses. Looking really great so far.
I asked quite a few questions around Qobj initialization. I'll continue the review of Field
and the other space classes when I can.
qutip/core/qobj.py
Outdated
and root_left * root_left != self._data.shape[1] | ||
): | ||
raise ValueError( | ||
"cannot build superoperator from nonsquare subspaces" |
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.
This error message seems inaccurate now.
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.
Not too sure how to properly explain it.
spre(NxM) @ spost(MxN)
is fine but spre(NxN) @ spost(MxM)
is not.
Both the input and output state of the super operator must be square matrix.
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.
Is the idea here that the constructor will automatically square the dims if a superoperator is requested and the dims of the operator are supplied?
If so could we simplify this check by checking self._data.shape[0] == self._dims.shape[0] ** 2
and self._data.shape[1] == self._dims.shape[1] ** 2
and then squaring the supplied dims?
If this is not exactly what happens, perhaps there is another way to simplify the check since the detected dims and the supplied data should be related somehow.
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.
If dims where not passed, dims where created to match the shape of the data.
If dims where passed, the check that their shape match the data shape was done in _initialize_data.
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.
Hmm. The maybe the issue is that the check is sort of split between here and _initialize_data
. Can we somehow make it so that the check happens in one place, or that it's repeated more fully here?
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.
It looks like the relevant code is gone, but I don't know where the check went. How did you decide to resolve this?
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 tried removing the type parameter and setter.
This mean that 1x1 are stuck at "scalar" without ways to change it and when creating super operators, setting dims properly is needed but were quite few places where it was not the case.
qutip/core/qobj.py
Outdated
and self._data.shape != dims.shape | ||
and self._data.shape == dims.shape[::-1] | ||
): | ||
self._data = _data.transpose(self._data) |
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.
Should we not raise some sort of error here if the shape of the data and the shape of the supplied dims don't match?
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.
It's for case where a vector is passed expecting a ket: Qobj([1, 2, 3], dims=[[3],[1]])
.
Rereading it, the check is too general...
Co-authored-by: Simon Cross <hodgestar+github@gmail.com>
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 now reviewed all of the new dimensions code and the QobjEvo and Qobj changes.
It would be good to have tests for the new dimensions classes (in this PR) and documentation (so that I can see what the classes are intended to do and so that users can work with them, either in this PR or a follow-up one).
In general the approach looks great.
|
||
@property | ||
def dims(self): | ||
return self._dims.as_list() |
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.
Should we also have a means for the user to access the actual dims object?
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.
To discuss, I do not like the list format much and would be fine with keeping it only for input and print.
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'd also be happy to only use the list format for input and printing the dims, but it would break some existing usage patterns (e.g. .dims[0]
). This is v5 though, so maybe that is okay. Perhaps something we can discuss in the admin meeting or on the mailing list?
qutip/core/qobj.py
Outdated
@@ -96,18 +82,16 @@ def _require_equal_type(method): | |||
@functools.wraps(method) | |||
def out(self, other): | |||
if other == 0: | |||
return method(self, other) | |||
return self.copy() |
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.
Calling .copy()
here seems wrong unless the method is something where 0
is the identity?
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.
_require_equal_type
is used for __add__
and __sub__
only.
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 don't think that is a good enough reason by itself to remove the call to method
since it would break potential other users and nothing about the name of the decorator suggests it is not generally applicable.
qutip/core/qobj.py
Outdated
and root_left * root_left != self._data.shape[1] | ||
): | ||
raise ValueError( | ||
"cannot build superoperator from nonsquare subspaces" |
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.
Is the idea here that the constructor will automatically square the dims if a superoperator is requested and the dims of the operator are supplied?
If so could we simplify this check by checking self._data.shape[0] == self._dims.shape[0] ** 2
and self._data.shape[1] == self._dims.shape[1] ** 2
and then squaring the supplied dims?
If this is not exactly what happens, perhaps there is another way to simplify the check since the detected dims and the supplied data should be related somehow.
qutip/core/dimensions.py
Outdated
self.from_ = from_ | ||
self.to_ = to_ | ||
self.shape = to_.size, from_.size | ||
self.issuper = from_.issuper or to_.issuper |
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.
Shouldn't it be an error is only one of from_.issuper
or to_.issuper
is true?
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.
Not sure if we should block this.
There are users who use rectangular (super)-operator that I don't understand.
Maybe this could allow to define measurement operator that take dm and return ket etc.
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.
My concern here is that .issuper
is either true or false, but it's not clear to me whether something with from_.issuper is True
and to_.issuper is False
is actually a super operator or not.
Co-authored-by: Simon Cross <hodgestar+github@gmail.com>
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.
@Ericgig Thanks for making the many rounds of changes and improvements. I'm pretty happy with how things look now and all the changes have made reviewing quite tricky. I propose that we merge this PR once you're happy and then make smaller issues for anything we want to clean up.
I'm happy to have another look at any final commits or things you're worried about in this PR though. Just ping me in the comments.
Description
This is an overhaul of the dimensions based on the proposition by @jakelishman in #1476, replace #1826, which was done before merging
master
intodev.major
and had conflicts.As proposed, dimension objects singleton instances composed of map and spaces.
Dimensions used by Qobj are all represented by a map between 2 spaces:
This Dimension is the
Map
proposed by Jake, but ket are map fromField
toSpace
and notSpace
themselves. Spaces come into multiple sub classes as proposed :Space
,SuperSpace
,Field
,Compound
, andEnrSpace
. Splitting maps and spaces simplify spaces by not needing to have a dummy 2nd dims.Dimensions can be initiated from a list and converted back to one. Field are automatically contracted:
Compound(Field, Field)
is aField
, same for maps. With the exceptions thatQobj
with a shape of(1,1)
with be aDimensions(Field, Field)
and not aField
.Some previously valid and used list format are no longer accepted. Playing with of tensor of super operators,
permute
andreshuffle
, you could make object with dims as[[[2, 2], [3, 3], [4, 4]], [[2, 2], [3, 3], [4, 4]]]
which can be interpreted as the tensor of 3 super operators. But then[[[2, 2], [3, 3]], [[2, 2],[3, 3]]]
can both be interpreted as tensor of super operator or super operator of tensor spaces, so :[[[2, 2], [3, 3], [4, 4]], [[2, 2], [3, 3], [4, 4]]]
: Error[[[2, 2], [4, 4]], [[2, 2], [4, 4]]]
: list representation of dimensions ofto_super(tensor(oper, oper))
[[[2], [2], [3], [3]], [[2], [2], [3], [3]]]
: list representation of dimensions oftensor(to_super(oper), to_super(oper))
Dimensions object have a
type
andshape
set a initialization. There is a check when creating/ modifying aQobj
with between the data's shape and dims's shape and an error is raise when they are not matching.Since operator-ket have a
superrep
, I added thesuperrep
parameter to the Superspace instead of the dimensions, for now. But a better look at super operator representation is needed.Qobj.dims
take and return a list, keeping the dimension object internally. From there, minimal adjustment were made so test pass.Related issues or PRs
Replace #1826