-
Notifications
You must be signed in to change notification settings - Fork 40
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
Conversation
…tion. No use of E.associates.
-- Eisenstein number @factor@ such that @(1 + ω)^powerUnit * e = 1 + 3*factor@. | ||
-- Note that L.findIndex cannot return Nothing. This happens only if @e@ is not | ||
-- coprime with 3. This cannot happen since @U.splitOff@ is called just before. | ||
getPrimaryDecomposition :: EisensteinInteger -> (Integer, 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.
Could you possibly return (CubicSymbol, EisensteinInteger)
and make corresponding changes to extractPrimaryContributions
and cubicHelper
? This will be more natural representation than Integer
and Mod 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:
- Instead of relying on autoderived
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.
instance Semigroup CubicSymbol where
(<>) = ...
stimes k n = case (k `mod` 3, n) of ...
- Could you please implement
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.
Could you please rebase your branch atop of @@ -96,8 +96,7 @@ cubicReciprocity alpha beta = cubicSymbolHelper beta alpha
extractPrimaryContributions :: EisensteinInteger -> EisensteinInteger -> (EisensteinInteger, EisensteinInteger, CubicSymbol)
extractPrimaryContributions alpha beta = (gamma, delta, newSymbol)
where
- newSymbol = stimes contribution Omega
- contribution = j*m - i*m -i*n
+ newSymbol = stimes (j * m) Omega <> stimes (- m - n) i
m :+ n = A.quot (delta - 1) 3
(i, gamma) = getPrimaryDecomposition alphaThreeFree
(_, delta) = getPrimaryDecomposition beta
@@ -111,12 +110,12 @@ extractPrimaryContributions alpha beta = (gamma, delta, newSymbol)
-- Eisenstein number @factor@ such that @(1 + ω)^powerUnit * e = 1 + 3*factor@.
-- Note that L.findIndex cannot return Nothing. This happens only if @e@ is not
-- coprime with 3. This cannot happen since @U.splitOff@ is called just before.
-getPrimaryDecomposition :: EisensteinInteger -> (Integer, EisensteinInteger)
+getPrimaryDecomposition :: EisensteinInteger -> (CubicSymbol, EisensteinInteger)
-- This is the case where a common factor between @alpha@ and @beta@ is detected.
-- In this instance @cubicReciprocity@ will return @Zero@.
-- Strictly speaking, this is not a primary decomposition.
-getPrimaryDecomposition 0 = (0, 0)
-getPrimaryDecomposition e = (toInteger powerUnit, factor)
+getPrimaryDecomposition 0 = (Zero, 0)
+getPrimaryDecomposition e = (stimes powerUnit Omega, factor)
where
factor = unit * e
unit = (1 + ω)^powerUnit |
-- Note that the units in @ids@ are ordered in the following way: | ||
-- The i^th element of @ids@ is @(1 + ω)^i@ starting from i = 0@ | ||
-- That is the i^th unit counting anticlockwise starting with 1. | ||
findPowerUnit = elemIndex inverseRemainder ids |
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 define symb
(equal to stimes powerUnit Omega
), and then one can choose either e * 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
or factor = -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 both e * ω
and -e * ω
cannot have the above property. The issue is that I need to know powerUnit
modulo 6 rather that modulo 3. One solution could be to pattern match on remainder
and define both factor
and symb
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.
Looks great! Please write haddock comments to the exported enitities and ensure that they are correctly rendered ( |
Merged, thanks for your efforts! |
This draft pull request is addressing Step 1 of the issue #155 (Cubic reciprocity law)
To be addressed: