-
Notifications
You must be signed in to change notification settings - Fork 31
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
Exemplary to_next_multiple #805
Conversation
could also be put somewhere else
qupulse/utils/__init__.py
Outdated
else: | ||
#still return 0 if duration==0 | ||
return lambda duration: ExpressionScalar((quantum/sample_rate)\ | ||
*Max(-(-(ExpressionScalar(duration)*sample_rate)//quantum),min_quanta)\ |
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.
We should try to avoid relying on sympy internals.
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.
Somehow a functionality that evaluates the min_quanta within the Expression has to be implemented. I did not directly see a different way than to use some sympy-functionality within the expression. Or are you just referring to the fact that this should be written out in e.g. a string to initialize the ExpressionScalar with?
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.
f'max({min_quanta}, {ceiled_division(duration, quantum)}) * {quantum}'
could work although I am not sure how robust the interaction with TimeType is.
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.
This seems to indeed be a bit problematic with TimeType (the tests give me weird behavior at the moment with a similar string), although I do not fully understand why.
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.
7c520a7 might resolve this; at least the test cases seem to work
could also be put somewhere else.
(Tests won't work here because floordiv not yet implemented in this branch)