Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Begin adding fmpz_mod_poly #87
Begin adding fmpz_mod_poly #87
Changes from 9 commits
fea7def
6b1611a
6797150
e6b28d4
4e64490
36724e3
0a84867
6122937
4827066
00b87f5
a3fec08
586307a
b2e2894
b060b23
c30ad7a
a2a7c1f
857ee26
9cabaa2
024a09c
9a01c5d
87a77b7
85f82a4
428bece
bc85cd3
cb60fe7
e57a071
69b2e50
a793997
8fd39d1
3614a42
9dfd197
1945859
57b8346
6a3b447
52dee64
b2543b4
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
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.
I guess we need to decide what this is supposed to do. I would also include a
repr(f)
test for the current behaviour along with a comment indicating that maybe the behaviour should be changed.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.
Yeah I'm not really happy with even the
repr(fmpz_mod(11, ctx))
output at the moment, I think it's a bit bulky and just outputting11
would be nicer on the eyes? I'm not sure what to do.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.
There is a bit of inconsistency:
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.
Personally I think that
repr
should show what representation you have like:Basically that is the purpose of
repr
in my mind. When you want a nicer dispaly you should use print/str. The awkward thing is that some builtin types likelist
will always use__repr__
recursively:One way to control this is just to have
repr
always show the pretty form but then that defeats the point ofrepr
in general. Another way is to have printer control for example the context settings could be used. Another way is just to take control of printing in the console (this is what SymPy and Sage do).I think it is something that should be decided on in general for python-flint and we should have a separate issue to discuss it.
For now I would just make sure that there is a test covering repr with its current behaviour along with a comment noting that the tested output is potentially not ideal.
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.
Yeah at the moment for the polynomial
repr
is the same asstr
but forfmpz_mod
we have a more verboserepr
compared to thestr
method. I think I prefer whatstr
does.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.
What
str
does is nice for the situation where it is intended: pretty printing.What
repr
should do is intended for a different situation. It is very often useful to know that you have anfmpz
rather than anint
or that yourx^2 - 1
is inF(163)[x]
rather thanZ[x]
, and is a python-flint type rather than a Sage/SymPy type etc.