Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Unified Primitive Container Types #53
Unified Primitive Container Types #53
Changes from 8 commits
5044117
9b38ea8
5bfb392
76dbfc5
9766e13
bfce062
124f6b2
a317b81
a791530
073f485
a1e4827
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 this implying that all subclasses of
TaskResult
must have exactly two stateful attributes:data
andmetadata
? I think the implication of the above paragraphs is "yes", but it might be worth spelling out in the interface. It leads to some tricks, though - if the intent is to leave the class open to later expansion, the allowance of subclasses to define arbitrary methods gets in the way; without explicitly reserving other names, there's no safe point for expansion.If the answer is instead "no", what's the intended purpose of forcibly putting
metadata
in a separate namespace?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'm having trouble understanding. This is not how I'd actually do it, but close enough for discussion:
I'm not sure how having a
get_counts()
method, for example, would get in the way.Is the alternative being considered here just putting the contents of metadata as attributes on
TaskResult
? This would mean that every implementation of Estimator would need its ownEstimatorTaskResult
in order to configure its own possible metadata values. It would also be quite annoying for workflow for IBM primitives, because if you wanted to modify the allowed metadata, you'd have to get the change into a tagged release ofqiskit_ibm_runtime
, then you'd need to get the tagged release to be the default version on the server-side, then you could do the thing you wanted to.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.
Super super minor, but Python's
Sequence
requires a few extra methods that we maybe don't want to actually imply -index
andcount
.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.
That's a good point, I don't want cluttering methods in the classes if we can avoid it. We can chat later about protocols.
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 kind of annoying how Sequence is something close but not equal to the thing everyone wants it to be.