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.

Sign up for GitHub

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

Jump to bottom

Feature: Allow scientific notation for literals #1721

Merged
merged 7 commits into from
Dec 6, 2019
Merged

Feature: Allow scientific notation for literals #1721

merged 7 commits into from
Dec 6, 2019

Conversation

fubuloubu
Copy link
Member

@fubuloubu fubuloubu commented Nov 12, 2019 •
edited
Loading

What I did

Actual solution to #1680, allowing scientific notation literals (e.g. 3e6, 1e-10) to be used in vyper programs.
See: https://python-reference.readthedocs.io/en/latest/docs/float/scientific.html

Work remaining

  • Need to work out semantics. Does a positive exponent get translated to an integer literal, or remain a decimal? Should it always be parsed a decimal? Parsing as a decimal always works
  • Needs significant test cases to prove equivalency of constant expressions to their runtime equivalent (avoid issues like Modulo at compile time differs from modulo at runtime #1693) Increased precision in fde6d2b avoids floating point issues

Description for the changelog

Added support for decimal literals using scientific notation

Cute Animal Picture

Hamster

@fubuloubu fubuloubu added the work in progress Work on this PR or issue is not yet complete but reviewers are free to add their input for guidance. label Nov 12, 2019
@fubuloubu fubuloubu force-pushed the feat-scientific-notation branch 3 times, most recently from b4b35b1 to f5473ad Compare November 12, 2019 19:07
@fubuloubu fubuloubu force-pushed the feat-scientific-notation branch 3 times, most recently from 9376f9b to 94b01e3 Compare November 21, 2019 15:06
@fubuloubu
Copy link
Member Author

Note: 3 of the failures are from #1681, looks like a few others are falling out due to something about using Decimal. The raw conversion cases look problematic however.

@charles-cooper
Copy link
Member

So I took a look at the failing tests and it looks like this PR is somehow picking up bad literals that were previously passing. Which I think is a good thing. I want to look at each one of them in more detail though.

@fubuloubu
Copy link
Member Author

Yeah, my thoughts exactly. It seems like there was some fallout of this change, which is good we're catching it now. I'll fix the lint issue.

@fubuloubu fubuloubu force-pushed the feat-scientific-notation branch 3 times, most recently from 216d6bb to 602e949 Compare November 29, 2019 00:58
@fubuloubu
Copy link
Member Author

I think I'm getting this tracked down... looks like the following phrase could be the cause?

"Substituting negative values for unary subtractions"

Perhaps on this substitution, there is something about constants where the unary substraction isn't being respected, ergo constant folding ignores the negative sign.

@fubuloubu fubuloubu mentioned this pull request Nov 29, 2019
Closed
@fubuloubu fubuloubu force-pushed the feat-scientific-notation branch 2 times, most recently from 2fa2bfe to 03bf59c Compare November 29, 2019 16:45
@fubuloubu
Copy link
Member Author

G2G once #1741 is merged (will rebase then)

@fubuloubu
Copy link
Member Author

Actually, scratch that... missing test cases!

@fubuloubu
Copy link
Member Author

Waiting for 2 reviews

@fubuloubu
feat: Added Decimal literal parsing
a0a1814
@fubuloubu
Revert "test: Added failing test cases from vyperlang#1680"
6cd1e7d 
This reverts commit fd9f3d0.
@fubuloubu
refactor: Convert without using while loop
53cbb70
@fubuloubu
bug: Latent failure on literal evaluation
01d453a 
Note: when using literals, it used to be acceptable to have >10
      decimal places, giving the false impression that you could
      specify a decimal literal as having >10 decimal places. It
      would silently drop the extra digits in that case.

Note: changed test case to gwei from szabo to "fix" the new
      conversion error
@fubuloubu fubuloubu removed the work in progress Work on this PR or issue is not yet complete but reviewers are free to add their input for guidance. label Dec 4, 2019
@fubuloubu
Copy link
Member Author

Whoops, forgot to remove the WIP label. Ready for review!

charles-cooper
charles-cooper approved these changes Dec 4, 2019
View reviewed changes
Copy link
Member

@charles-cooper charles-cooper 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 pretty happy with this. Thanks!

jacqueswww
jacqueswww approved these changes Dec 6, 2019
View reviewed changes
Copy link
Contributor

@jacqueswww jacqueswww left a comment

Choose a reason for hiding this comment

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

LGTM

@fubuloubu fubuloubu merged commit 609c339 into vyperlang:master Dec 6, 2019
@fubuloubu fubuloubu deleted the feat-scientific-notation branch December 6, 2019 16:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@charles-cooper charles-cooper charles-cooper approved these changes

@jacqueswww jacqueswww jacqueswww approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@fubuloubu @charles-cooper @jacqueswww