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

cannot unify numpy.int? with float #653

Closed
jbqubit opened this issue Jan 11, 2017 · 9 comments
Closed

cannot unify numpy.int? with float #653

jbqubit opened this issue Jan 11, 2017 · 9 comments

Comments

@jbqubit
Copy link
Contributor

jbqubit commented Jan 11, 2017

from artiq.experiment import *


class SAWGTest(EnvExperiment):
    def build(self):
        self.setattr_device("core")
        self.setattr_device("sawg0")


    @kernel
    def run(self):
        self.core.reset()
        delay(10*ms)
        self.sawg0.phase1.set(0)
repository/bug.py:14:31-14:32: error: cannot unify numpy.int? with float
        self.sawg0.phase1.set(0)
                              ^ 
repository/bug.py:14:31-14:32: note: expression of type numpy.int?
        self.sawg0.phase1.set(0)
                              ^ 
@jordens
Copy link
Member

jordens commented Jan 11, 2017

The argument is a float. Give it a float.
1 turn = 1 = 1*s*Hz = 1*s*s*Hz*Hz. It's unitless.

@jordens jordens closed this as completed Jan 11, 2017
@jbqubit
Copy link
Contributor Author

jbqubit commented Jan 11, 2017

  • This is inconsistent. I can set a frequency 50*MHz.
  • Since when is phase unitless? Common units are degrees, radians, turns. ARTIQ is explicit about V, s, A, etc.

@jordens
Copy link
Member

jordens commented Jan 11, 2017

No. It's not inconsistent 50*MHz is a float. It is 50*1e6.
You first claim 1*turn=2pi. What unit does that have? Both sides?
Phase has units implied by context or convention: people are allowed to write 2*pi in papers as a phase. pi has no units.

@jbqubit
Copy link
Contributor Author

jbqubit commented Jan 11, 2017

No. It's not inconsistent 50MHz is a float. It is 50e6.

It implicitly becomes a float after unit processing. The same happens in the following example too.

self.sawg0.frequency0.set(50000000)

You first claim "1turn=2pi". What unit does that have? Both sides?

OK I was imprecise. 0.5 turn is 1*pi rad is 180 deg.

@jbqubit
Copy link
Contributor Author

jbqubit commented Jan 25, 2017

@whitequark Thoughts on this?

@whitequark
Copy link
Contributor

@jbqubit It is possible to implement automatic coercions for argument passing in ARTIQ Python, but due to the way type inference works, it's a zero-sum game: for every case where I add a possible coercion, there will be another case where a type annotation is now mandatory. Especially since that will break existing code, I think this feature is a net negative, and in any case the added effort would seem to be higher than fixing the type of a numeric constant.

@jbqubit
Copy link
Contributor Author

jbqubit commented Jan 26, 2017

@whitequark Ya, mandatory type annotation isn't desirable. Thanks for the comment.

@jordens
Copy link
Member

jordens commented Jan 26, 2017

@jbqubit

  • self.sawg0.frequency0.set(50000000) does not work in exactly the same way as phase0.set(0) does not work.
  • You have a skewed view of what units.py does and what it can do. This is purely syntactic sugar and readability and not -- as you seem to think -- a unit system that does things like conversions for you.
  • 0.5*turns can not be 1*pi with our unit system. We would have to redefine pi or use rad as the "default" phase.

@whitequark
Copy link
Contributor

whitequark commented Jan 26, 2017

@jbqubit To add to this, automatic coercions for arithmetic operations already result in necessity for type annotations in some cases. E.g. this function on its own is ambiguous:

def add(x, y):
  return x + y

This is not a very large problem right now because passing a variable with a known type, say a field, to such a function monomorphizes it. But if every argument were subject to coercion, that would become impossible.

Sorry, something went wrong.

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

No branches or pull requests

3 participants