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
Draft of CubicSymbol #194
Draft of CubicSymbol #194
Changes from 13 commits
27feeae
8c7cb1e
ad0d206
72ce88f
d08fc9f
d9ea713
a05b4b6
97f4270
03b7df7
8ab434b
4730d19
eac987b
0533ff6
4043c6c
20ab1b8
b00093b
88a218d
eb9e5ee
c4ad23b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Could you possibly return
(CubicSymbol, EisensteinInteger)
and make corresponding changes toextractPrimaryContributions
andcubicHelper
? This will be more natural representation thanInteger
andMod 3
, I think.I suggest to pattern match on possible values of
remainder
and return corresponding cubic symbols directly.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 is probably better
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 changed it but I am disappointed by the result. The code looks messier and it runs more slowly. I thought about it but I could not find any better way of implementing these changes. If you have any suggestions to improve it, let me know.
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.
That's surprising, I'll take a look next week. I should revisit
M.NT.Q.EisensteinIntegers
to check that all operations are as efficient as possible.In the meantime I have three suggestions:
stimes
(which appears to be awkward) we can actually define our own, supporting zeros and negatives. This basically brings us back to your very first design, sorry for the digression.symbolToNum :: CubicSymbol -> EisensteinInteger
?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 did this. In the new commit, the timings went down to the previous level.
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.
It seems we can do this "table lookup" more explicit, pattern-matching by all possible values of
remainder
. Pattern-matching provides an elegant way to definesymb
(equal tostimes powerUnit Omega
), and then one can choose eithere * symbolToNum symb
or-e * symbolToNum
, depending on which of them is equivalent to 1 modulo 3.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.
As I understand it, this method is not going to work. My understanding is that you want to define
factor = e * symbolToNum symb
orfactor = -e * symbolToNum symb
depending on which one is equivalent to 1 mod 3. In general, I don't think that either of them need be equivalent to 1 mod 3. For instance, suppose that I find that(1 + ω) * e A.rem 3 = 1
. Then its associated cubic symbol is ω. Then you see that bothe * ω
and-e * ω
cannot have the above property. The issue is that I need to knowpowerUnit
modulo 6 rather that modulo 3. One solution could be to pattern match onremainder
and define bothfactor
andsymb
at the same time. Let me know what you think.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 have taken this approach in the latest commit. I would like to make the pattern matching more readable but the program does not compile if I use ω when I list the cases. Also, it gives me a warning for redundant pattern matching which I don't agree with.