Skip to content

Conversation

braised-babbage
Copy link
Contributor

@braised-babbage braised-babbage commented Nov 4, 2020

EYE wraps MAGICL:EYE, which may be provided with both a type and a value for the diagonal. Currently, MAGICL:EYE will COERCE the value to the type, if need be. However, (complex double-float) cannot be coerced to double-float, even if the imaginary part is zero. Every so often quilc tries to construct a double-float identity matrix, e.g. here https://github.com/rigetti/quilc/blob/master/src/compilers/approx.lisp#L201, and this will cause an error.

@braised-babbage braised-babbage requested a review from a team as a code owner November 4, 2020 19:25
colescott
colescott previously approved these changes Nov 4, 2020
@notmgsk notmgsk requested a review from stylewarning November 4, 2020 19:58
@braised-babbage
Copy link
Contributor Author

Original commit was the wrong thing, since MAGICL:EYE will prioritize the type of :VALUE over a user-specified :TYPE. It's better to just not use any default value here.

@braised-babbage braised-babbage changed the title Use 1d0 as the default value for MAGICL:EYE Do not use a default value in EYE Nov 4, 2020
Copy link
Member

@colescott colescott left a comment

Choose a reason for hiding this comment

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

It might be good to remove the other defaults in this file that are just copies of the magicl defaults to avoid confusion like this in the future. (L34,40,46)

I think the original purpose for leaving those in is to allow the caller to see what the default will be, however this might not be worth the added weirdness.

@notmgsk notmgsk merged commit 143629c into quil-lang:master Dec 2, 2020
@notmgsk notmgsk mentioned this pull request Dec 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants