Skip to content
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

Specifics for casting classical types #268

Merged
merged 20 commits into from
Dec 1, 2021

Conversation

mbhealy
Copy link
Contributor

@mbhealy mbhealy commented Aug 31, 2021

Summary

This PR represents the first round of work from the Types and Casting WG. It contains specifics for the expected behavior when casting between classical types.

Details and comments

Many mentions are made to slicing being required when there is precision mismatch in a casting operation. Slicing syntax is going to be discussed soon by the same WG, but has not been included in this PR.

Pursuant to PR #267 I have not included anything related to the fixed type.

I also modified the duration type to allow negative values, as the WG has discussed. I added the capability to divide durations by other durations, floats, and consts, this returns a float value. This is the alternative to allowing casting of durations, which we weren't able to come up with a well-defined behavior for.

@mbhealy mbhealy self-assigned this Aug 31, 2021
@mbhealy mbhealy added TSC Technical Steering Committee WG: types-and-casting Working Group: types-and-casting labels Aug 31, 2021
Copy link
Contributor

@jakelishman jakelishman left a comment

Choose a reason for hiding this comment

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

Thanks for writing it! It looks good, and the table is helpful. (Surprised we need jQuery to get the backgrounds, but that's a complete non-issue, really.)

I left a couple of very minor wording changes, and a few questions as to whether we should include a little more information in places, but those don't necessarily need action - they're just queries. My main point is about the duration division thing - I had thought the result of the discussion was to define duration / duration -> float and leave duration / float -> duration. Certainly division by float seems much more logical to me to produce a duration not a float, when looking at written-down examples - if it produces a float, it would be weird to me duration * float works "as expected" mathematically but duration / float changes the type.

source/language/delays.rst Outdated Show resolved Hide resolved
source/language/types.rst Outdated Show resolved Hide resolved
source/language/types.rst Outdated Show resolved Hide resolved
source/language/types.rst Outdated Show resolved Hide resolved
source/language/types.rst Outdated Show resolved Hide resolved
source/language/types.rst Show resolved Hide resolved
source/language/types.rst Outdated Show resolved Hide resolved
source/language/types.rst Outdated Show resolved Hide resolved
@mbhealy mbhealy marked this pull request as ready for review September 3, 2021 13:13
@jwoehr
Copy link
Collaborator

jwoehr commented Sep 7, 2021

PR reads:

In general, for any cast between standard types that results in loss of
precision, if the source value is larger than can be represented in the target
type, the exact behavior is implementation specific.

Suggestion:

the exact behavior is implementation specific and must be documented by the vendor.

@jwoehr
Copy link
Collaborator

jwoehr commented Sep 7, 2021

int[n] and uint[n] values cast to the standard types mimicking C
behavior.

This may need precise definition without waving at C, whose behavior in this regard has changed somewhat from time to time over 50 years.

@jwoehr
Copy link
Collaborator

jwoehr commented Sep 7, 2021

float[n] values cast to the standard types mimicking C behavior (i.e.
discarding the fractional part for integer-type targets).

Similarly, without waving at C. And I believe e.g. is meant here unless the only transformation in casting floats is the discarding of the fractional part, which does not appear to be the case here as floats can be cast to angle.

@jwoehr
Copy link
Collaborator

jwoehr commented Sep 7, 2021

Why can't an int or uint be cast to an angle, and back again?

@mbhealy
Copy link
Contributor Author

mbhealy commented Sep 7, 2021

Why can't an int or uint be cast to an angle, and back again?

Angles are interpreted as a stored value multiplied by 2pi, The stored value therefore exists in the range [0,1.0), that value will always cast to 0 as an int/uint. Therefore, we decided that the cast made no sense. We think it is better for intermediate types to be used to manipulate angle values using int/uint logic. For example, an angle value can be cast to a bit of the same width to get an exact bit-level copy, which can then be cast to an int/uint.

One could argue that the value in the range (0,2*pi] could be cast to an int in the range (0,6), but this seemed to have dubious utility to the WG.

@mbhealy
Copy link
Contributor Author

mbhealy commented Sep 7, 2021

float[n] values cast to the standard types mimicking C behavior (i.e.
discarding the fractional part for integer-type targets).

Similarly, without waving at C. And I believe e.g. is meant here unless the only transformation in casting floats is the discarding of the fractional part, which does not appear to be the case here as floats can be cast to angle.

Good point, I have updated this to specify the C99 standard behavior in d4b1f82

I would like to note that angle is an OpenQASM special type (along with duration, bit, and qubit), and not a 'standard' type, which consists of bool, int, uint, and float. Perhaps we should document this difference in the spec?

@jwoehr
Copy link
Collaborator

jwoehr commented Sep 7, 2021

Good point, I have updated this to specify the C99 standard behavior in d4b1f82

Noted, and excellent.

I would like to note that angle is an OpenQASM special type (along with duration, bit, and qubit), and not a 'standard' type, which consists of bool, int, uint, and float. Perhaps we should document this difference in the spec?

A concise note cannot hurt!

@mbhealy
Copy link
Contributor Author

mbhealy commented Sep 7, 2021

the exact behavior is implementation specific and must be documented by the vendor.

Another good point, added in 0a839ad

@mbhealy
Copy link
Contributor Author

mbhealy commented Sep 7, 2021

I would like to note that angle is an OpenQASM special type (along with duration, bit, and qubit), and not a 'standard' type, which consists of bool, int, uint, and float. Perhaps we should document this difference in the spec?

A concise note cannot hurt!

Added in f11982d

@@ -290,9 +291,10 @@ Duration
~~~~~~~~

We introduce a ``duration`` type to express timing.
Durations are positive numbers with a unit of time. ``ns, μs, ms, s`` are used for SI time
Durations are numbers with a unit of time. ``ns, μs, ms, s`` are used for SI time
Copy link
Contributor

Choose a reason for hiding this comment

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

We should also allow "us" as a synonym for "μs"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added in 6088c02

Casting from int/uint
~~~~~~~~~~~~~~~~~~~~~

``int[n]`` and ``uint[n]`` values cast to the standard types mimicking C99
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we be explicit about the int -> bool cast here? Similar to the other types

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added in 00bf4e7

@@ -17,6 +17,7 @@ override a reserved identifier.
In addition to being assigned values within a program, all of the classical
types can be initialized on declaration. Any classical variable or Boolean that is not explicitly
initialized is undefined. Classical types can be mutually cast to one another using the typename.
See :ref:`castingSpecifics` for more details on casting.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe I missed this, but is there a specific casting syntax? Or do you initialize a new variable with that type?

Similarly, do casts happen automatically when the wrong type is returned from a function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Casting syntax is listed in the current spec, see the example at the end of Integers
It looks like:

type1 identifier1 = initialization;
type2 identifier2 = type2(identifier1);

The type() syntax is the casting syntax

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Similarly, do casts happen automatically when the wrong type is returned from a function?

This counts as implicit casting, and we are working on defining rules for that now. It is explicitly disallowed in the current spec as far as I understand.

@mbhealy
Copy link
Contributor Author

mbhealy commented Nov 10, 2021

@mbhealy I was able to change the words extended language in the first sentence of source/language/index.rst to purple using css.

@jwoehr Thank you very much for the pointers. I was able to use the css version you pointed out to color the text in the same manner (12af938). Unfortunately the centering doesn't seem to be working correctly, but it still looks pretty good.

@jwoehr
Copy link
Collaborator

jwoehr commented Nov 11, 2021

Unfortunately the centering doesn't seem to be working correctly, but it still looks pretty good.

@mbhealy I see that the custom css

table.align-center {
    margin-left: auto;
    margin-right: auto;
}

was recommended in the last response to the 2017 Sphinx issue 3942

@mbhealy
Copy link
Contributor Author

mbhealy commented Nov 11, 2021

Unfortunately the centering doesn't seem to be working correctly, but it still looks pretty good.

@mbhealy I see that the custom css

table.align-center {
    margin-left: auto;
    margin-right: auto;
}

was recommended in the last response to the 2017 Sphinx issue 3942

Yes, I attempted that but it did not work.

@blakejohnson
Copy link
Contributor

@mbhealy the revised styling seems much less intrusive in the doc. Thanks.

@blakejohnson
Copy link
Contributor

So, all that's left now is to resolve this discussion: https://github.com/Qiskit/openqasm/pull/268/files#r732623768

@mbhealy
Copy link
Contributor Author

mbhealy commented Nov 11, 2021

So, all that's left now is to resolve this discussion: https://github.com/Qiskit/openqasm/pull/268/files#r732623768

The TG decided in today's meeting to simply remove the possibility of casting angles to floats. Changed in 0427362

The TG decided that we should push off the enablement of casting angles to floats for a future update.

Copy link
Contributor

@jakelishman jakelishman left a comment

Choose a reason for hiding this comment

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

In general I'm pretty happy with this, but I read through it again and got myself in a right tizzy about what happens when we're assuming "little-endian ordering" in bit-string conversions. Could someone check and potentially point out the trivial mistake I'm making?

I left a couple of other comments about places we could tighten the language/spec, but I'm happy to accept the text with or without them for now.

source/language/types.rst Outdated Show resolved Hide resolved
Comment on lines +447 to +448
``float[n]`` values cast to the standard types mimicking C99 behavior (*e.g.*
discarding the fractional part for integer-type targets). As noted above,
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't mention the val != 0.0 convention for Boolean conversion here. Not certain it's necessary to say it again (we could just move all instances of it up to the top paragraph), but an interesting consequence of this convention is that bool(nan) == true, because nan compares unequal to everything. That's consistent with C and Python, for sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I feel like it's reasonable to rely on the C99 spec being generally well understood in this regard and also easily accessible to programmers, but I'm willing to add it if other agree that it's better.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, agreed (we don't need to add more text). Mostly I was just curious about what happened with nan! The val != 0 seems to be entirely consistent with other languages, which also have bool(nan) is true.

source/language/types.rst Show resolved Hide resolved
source/language/types.rst Outdated Show resolved Hide resolved
source/language/types.rst Show resolved Hide resolved
jakelishman
jakelishman previously approved these changes Nov 12, 2021
Copy link
Contributor

@jakelishman jakelishman left a comment

Choose a reason for hiding this comment

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

I'm happy to accept this PR as-is, regardless of my other comments. Thank you for all the effort, Michael!

blakejohnson
blakejohnson previously approved these changes Nov 12, 2021
jakelishman
jakelishman previously approved these changes Nov 12, 2021
ajavadia
ajavadia previously approved these changes Nov 15, 2021
ndebeaudrap
ndebeaudrap previously approved these changes Nov 15, 2021
stevenheidel
stevenheidel previously approved these changes Dec 1, 2021
@1ucian0 1ucian0 merged commit 2a3855f into openqasm:master Dec 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
TSC Technical Steering Committee WG: types-and-casting Working Group: types-and-casting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants