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

Examples: Fix compiling issues and imports, closes #69 #73

Merged
merged 16 commits into from
Jan 31, 2022

Conversation

dlesnoff
Copy link
Contributor

@dlesnoff dlesnoff commented Jan 9, 2022

Fix compilation errors for many examples.
Change import bigints to import ../src/bigints to ensure examples are tested with the development version and not the stable version. This might be changed back when a stable version is going to be released.
Changed imports from standard library to differentiate them from external libraries :
import strutils, math become import std/[strutils, math]
As there is no implicit type conversion for integers to BigInt anymore, operations between a BigInt and an Integer now become operations between BigInt and integer explicitly converted to BigInt.
Now three examples do not work :

  • pidigits.nim - rangeDefect
  • rc_pidigits.nim - rangeDefect (almost identical to pidigits.nim)
  • rc_godtheinteger.nim - infinite Loop (takes more than 30 seconds)
  • rc_leftfactorials.nim - depends on an external library iterutils.
  • rc_paraffins.nim

@pietroppeter
Copy link
Contributor

#69 (comment)

@dlesnoff
Copy link
Contributor Author

Sorry I have seen your comment after my PR, I am going to rebase my fix.

@pietroppeter
Copy link
Contributor

rc_godtheinteger.nim - infinite Loop (takes more than 30 seconds)

see http://rosettacode.org/wiki/9_billion_names_of_God_the_integer#Nim that version is supposed to be slow and as such should not be kept running. version 2 is the fast one.

@dlesnoff
Copy link
Contributor Author

I have :

  • changed imports back to import bigint
  • added a nim.cfg
  • in src/bigints.nim, there is now a ^ decorator for pow. Why was this removed ?
    It helps to keep some examples easy to read in my opinion.
  • I can not compile anymore rc_parafins.nim. It fails at line 42. I am unsure yet how to fix it.
  • rc_pidigits.nim and pidigits.nim are still broken, it would take me a lot of time to understand the algorithms and rewrite them.
  • @def- Is it possible to replace the slice iterator from iterutils package in your example, to remove iterutils dependency ?

@narimiran
Copy link
Member

@konsumlamm, your review please :)

@pietroppeter
Copy link
Contributor

pietroppeter commented Jan 10, 2022

Is it possible to replace the slice iterator from iterutils package in your example, to remove iterutils dependency ?

I would adapt rc_leftfactorials.nim it to something like this (playground):

iterator square: int =
  var i = 0
  while true:
    yield i*i
    inc i

var i = 0
for n in square():
  if i <= 10:
    echo i, ": ", n
  elif i <= 110 and i mod 10 == 0:
    echo i, ": ", n
  elif i >= 1000 and i <= 10_000 and i mod 1000 == 0:
    echo i, ": ", n
  elif i > 10_000:
    break
  inc i

@pietroppeter
Copy link
Contributor

in examples/rc_godtheinteger.nim you could change this line:

for x in [23, 123, 1234, 12345]:

to

# for 12345 this implementation is too slow, for a faster implementation see rc_godtheinteger2.nim
for x in [23, 123, 1234]:

@dlesnoff
Copy link
Contributor Author

Thanks a lot @pietroppeter, the two new commits follow up from your ideas.

@@ -20,16 +29,16 @@ proc extractDigit(): int32 =
if tmp2 >= den:
return -1

result = int32(tmp1.limbs[0])
Copy link
Contributor Author

@dlesnoff dlesnoff Jan 10, 2022

Choose a reason for hiding this comment

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

This example can not be rewritten as long as we have not defined a way to convert a BigInt into an int, see issue #72. Type cast does not do the same thing, it interprets the BigInt as an int ! So the first limb is understood as the 2’s complement representation of another number. Thanks @konsumlamm for the explanation. Same for examples/rc_pidigits.nim

Copy link
Contributor

@konsumlamm konsumlamm left a comment

Choose a reason for hiding this comment

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

In the future, we should also test these examples in the CI (I didn't even know they existed until recently).

src/bigints.nim Outdated Show resolved Hide resolved
examples/elliptic.nim Outdated Show resolved Hide resolved
examples/elliptic.nim Outdated Show resolved Hide resolved
examples/pidigits.nim Outdated Show resolved Hide resolved
examples/rc_godtheinteger2.nim Outdated Show resolved Hide resolved
examples/rc_modexp.nim Outdated Show resolved Hide resolved
@dlesnoff
Copy link
Contributor Author

All examples compile, only pidigits examples does not work but this may be fixed with PR #74.

@dlesnoff
Copy link
Contributor Author

Pidigits examples now compile, but in case toSignedInt change name, we will have to modify slightly the examples again.
In pidigits.nim, I added command line basic parsing. I thought it was broken, because I did not provide an argument, and nothing explained that an argument was expected.

@dlesnoff dlesnoff changed the title Examples: Fix compiling issues and almost all imports Examples: Fix compiling issues and imports, closes #69 Jan 16, 2022
@dlesnoff
Copy link
Contributor Author

@konsumlamm or @pietroppeter may I ask for a review on this PR ?

Copy link
Contributor

@konsumlamm konsumlamm left a comment

Choose a reason for hiding this comment

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

Can you change the link in examples/rc_godtheinteger.nim to https://rosettacode.org/wiki/9_billion_names_of_God_the_integer and change all links to https?

examples/elliptic.nim Outdated Show resolved Hide resolved
examples/rc_combperm.nim Outdated Show resolved Hide resolved
@@ -1,10 +1,19 @@
# Translation of http://benchmarksgame.alioth.debian.org/u32/program.php?test=pidigits&lang=go&id=4
Copy link
Contributor

Choose a reason for hiding this comment

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

This link is broken.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, isn't this example essentially a duplicate of rc_pidigits.nim?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The pidigits.nim file is the interactive version of rc_pidigits.nim
The pidigits.nim asks the user for the number of digits of pi he wants, whereas rc_pidigits.nim prints indefinitely new digits of pi.
I can remove rc_pidigits.nim if you want. Especially knowing that the code (now outdated) is published on the website.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't really care which one we keep.

examples/rc_modexp.nim Outdated Show resolved Hide resolved
@dlesnoff
Copy link
Contributor Author

@narimiran May I ask to merge this PR ?

@narimiran narimiran merged commit 7c9074a into nim-lang:master Jan 31, 2022
@dlesnoff dlesnoff deleted the fixExamples branch February 16, 2022 21:49
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