Skip to content

feat: include units for duration and angles#9

Merged
qartik merged 4 commits intomainfrom
units-for-angles-and-duration
Oct 23, 2023
Merged

feat: include units for duration and angles#9
qartik merged 4 commits intomainfrom
units-for-angles-and-duration

Conversation

@qartik
Copy link
Member

@qartik qartik commented Oct 20, 2023

Another change is that "duration" is now an optional field of MOp

Closes #6

@qartik qartik requested a review from qciaran October 20, 2023 22:00
@qartik qartik linked an issue Oct 20, 2023 that may be closed by this pull request
qartik added a commit to Quantinuum/pytket-phir that referenced this pull request Oct 20, 2023
@qciaran
Copy link
Collaborator

qciaran commented Oct 20, 2023

The solution is a little different then what I was thinking. It looks like pure floats are kept as pure floats and assumed to be radians and you have the option to include a "pi" unit; while, for durations it looks like you always have to specify the unit.

To be consistent between angles and durations, I was thinking more, if the user can choose to supply both angles and durations as purely floats and that the Model will recast it as [float, "rad"], [float, "s"], respectively. (That is, float == [float, "rad"], float == [float, "s"], respectively.) The user/generator of the JSON can also choose to be explicit with typing.

Is that a pain, e.g., for validation? Or do you think there are other issues with that? I kind of like your solution actually. The only thing that causes me pause is the inconsistency (but maybe they don't have to be consistent).

@qartik
Copy link
Member Author

qartik commented Oct 23, 2023

@qciaran I won't worry about consistency between angles and durations. For angles, as you argued, it makes sense to have a default as it's both obvious and widely accepted that without any units, angles are radians.

For durations, however, it's quite perplexing to not see any units attached while reading a number.

As a further improvement, I am considering the unit of angles to be mentioned only once in the QOp, so perhaps a field such as angles_unit that has the default value "rad" (or None representing radians) and other acceptable values like "pi" or even "deg". To me, it does not make sense to provide angles in multiple units for a single gate. Moreover, providing units with each angle makes it unnecessarily verbose. Do you foresee that being problematic?

For example, compare:

QOp(metadata=None, qop='PhasedX', returns=None, args=[('q', 0)],
    angles=[(0.5, 'pi'), (0.5, 'pi')]),

vs

QOp(metadata=None, qop='PhasedX', returns=None, args=[('q', 0)],
    angles=[0.5, 0.5], angles_unit="pi"),

@qciaran
Copy link
Collaborator

qciaran commented Oct 23, 2023

Since radian are usually given as unitless, that is fine. However, I would go with "rad" instead of None to be explicit. (I don't think we need to bother with "deg" since that isn't used in any scientific computing that I am aware of.) Sure consolidating the angles_unit to a separate entry is fine, although keeping them with the angle does not disturb me either. If you do go with a separate field for the unit, I think a consistent approach should usually be taken so you don't have to remember special cases. Therefore, if you go with angles_unit you should also have duration_unit.

@qartik
Copy link
Member Author

qartik commented Oct 23, 2023

Agree with everything, except forcing duration_unit as a separate field when the type Duration is sufficient to capture a duration fully. Perhaps a compromise might be better?

How about the following definition for angles?

angles: tuple[list[float], Literal("rad", "pi")] | None = None

@qciaran
Copy link
Collaborator

qciaran commented Oct 23, 2023

Yeah, that works for me.

@qartik qartik added the enhancement New feature or request label Oct 23, 2023
@qartik
Copy link
Member Author

qartik commented Oct 23, 2023

@qciaran ready for your review. Thanks!

qartik added a commit to Quantinuum/pytket-phir that referenced this pull request Oct 23, 2023
Copy link
Collaborator

@qciaran qciaran left a comment

Choose a reason for hiding this comment

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

Looks great!

@qartik qartik merged commit 760d4f3 into main Oct 23, 2023
@qartik qartik deleted the units-for-angles-and-duration branch October 23, 2023 16:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Include units for angles and transport duration in the spec

2 participants