Skip to content

Remove long/int relic#37420

Merged
vbraun merged 5 commits intosagemath:developfrom
nbruin:patch-2
Mar 31, 2024
Merged

Remove long/int relic#37420
vbraun merged 5 commits intosagemath:developfrom
nbruin:patch-2

Conversation

@nbruin
Copy link
Copy Markdown
Contributor

@nbruin nbruin commented Feb 22, 2024

in py2, long and int were different types. In py3, only int remains, with long a synonym in cython.

📝 Checklist

  • The title is concise, informative, and self-explanatory.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation accordingly.

⌛ Dependencies

@nbruin
Remove long/int relic
81a71ae 
in py2, long and int were different types. In py3, only int remains, with long a synonym in cython.
@videlec
Copy link
Copy Markdown
Contributor

videlec commented Feb 22, 2024

Wouldn't it make sense to get rid of sage.rings.integer.int_to_Z? (actually I would get rid of it and then rename long_to_Z -> int_to_Z)

@nbruin
remove int_to_Z because it's buggy
33e14a9 
int_to_Z used to work in Py2 on the type "int" there which were integers that fit in a word. In Py3 there is no such type anymore and "int" now is what "long" used to be -- -arbitrary length integers. In Py3 int_to_Z is buggy:

sage: itoZ=sage.rings.integer.int_to_Z()
sage: itoZ(1000000000000000000000)
OverflowError
@nbruin
remove forward declaration of int_to_Z
fa1905c
@nbruin
Copy link
Copy Markdown
Contributor Author

nbruin commented Feb 22, 2024

Yes, it's worse. int_to_Z still exists but is buggy for what int is in Py3. Removed the definition to see what happens. Will probably rename long_to_Z to int_to_Z in the next step (with that change hopefully we don't have to change any code elsewhere)

@videlec
Copy link
Copy Markdown
Contributor

videlec commented Feb 22, 2024

Maybe keep an alias long_to_Z = int_to_Z to avoid backward incompatibility?

@nbruin
Copy link
Copy Markdown
Contributor Author

nbruin commented Feb 22, 2024

well ... int_to_Z was exported for cimportin the pxd but removing that didn't cause any test failures, so I guess it's not used. Indeed, these coercion morphisms should only be used for the coercion system. Any time-critical code can use much more direct conversion routines.

I guess in cython long and int are synonyms (as far as python types are concerned), but in py3, only int remains, so I guess we should rename. Another round of tests will tell us the call sites ...

@github-actions
Copy link
Copy Markdown

github-actions bot commented Feb 22, 2024

Documentation preview for this PR (built with commit 18ace4e; changes) is ready! 🎉

@nbruin
Copy link
Copy Markdown
Contributor Author

nbruin commented Feb 22, 2024

OK, testing seems successful. Ready to be merged, I think. Review?

grhkm21
grhkm21 approved these changes Feb 23, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@grhkm21 grhkm21 left a comment

Choose a reason for hiding this comment

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

Looks good to me, cleaning up code is always nice.

@grhkm21
Copy link
Copy Markdown
Contributor

grhkm21 commented Feb 23, 2024

And I agree that keeping (alias) long_to_Z is not necessary. Looking through the 244 usages of long_to_Z on GitHub I see all of them are just forks on Sage, so it's okay.

Also there's a function PyLong_to_ZZ still in Sage, but that's correct to keep since PyLong is a CPython thing(?)

@nbruin
Copy link
Copy Markdown
Contributor Author

nbruin commented Feb 23, 2024 •
edited
Loading

Also there's a function PyLong_to_ZZ still in Sage, but that's correct to keep since PyLong is a CPython thing(?)

Yes I think it is:
https://docs.python.org/3/c-api/long.html#c.PyLongObject

tornaria added a commit to tornaria/sage that referenced this pull request Mar 31, 2024
@tornaria
Make this PR mergeable with sagemath#37420
867ece7
@tornaria tornaria mentioned this pull request Mar 31, 2024
Merged
3 tasks
@vbraun vbraun merged commit 4796e74 into sagemath:develop Mar 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@grhkm21 grhkm21 grhkm21 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

p: trivial / 5

Projects

None yet

Milestone

sage-10.4

Development

Successfully merging this pull request may close these issues.

5 participants

@nbruin @videlec @grhkm21 @vbraun @mkoeppe