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

Consolidate the series code #992

Closed
thofma opened this issue Feb 21, 2021 · 9 comments
Closed

Consolidate the series code #992

thofma opened this issue Feb 21, 2021 · 9 comments

Comments

@thofma
Copy link
Member

thofma commented Feb 21, 2021

At the moment working on the series code is rather complicated, as there is a lot of code duplication. Given that we want to add gfp_*, this will even get worse. Here are two proposals on how to deal with it.

  1. Metaprogramming. We wrap everthing in for T in [fmpz, fmpq, ...]; @eval .... Has the big disadvantage that adding methods for specific types is more complicated.

  2. Parametrize the wrapper and have

    mutable struct NemoAbsSeries{S, T} <: AbsSeries{S}
      poly::T
      ...
    end

    And similar for NemoRelSeries{S, T}. This is basically the same that we did for the Laurent series.

    With type aliases fmpz_abs_series = NemoAbsSeries{fmpz, fmpz_poly} this is even non-breaking and with julia version >= 1.6 this will even be printed as fmpz_abs_series (see RFC: print all types using equivalent exported aliases JuliaLang/julia#36107).

    This makes also trivial to add further types and/or adding methods to functions for specific types.

Opinions? I personally prefer option 2). If we can agree on something, I am happy to implement it.

@wbhart
Copy link
Contributor

wbhart commented Feb 21, 2021

I personally prefer option 2, so have at it.

But I think you are going to find Jit time increase the more metaprogramming you do.

@thofma
Copy link
Member Author

thofma commented Feb 21, 2021

Option 2) should not require any metaprogramming, but I am happy to also make some timings for compile time.

@thofma
Copy link
Member Author

thofma commented Feb 21, 2021

Ah sorry, maybe it will, you are right. I will give it a try.

@wbhart
Copy link
Contributor

wbhart commented Feb 21, 2021

You also have to remember that the Z/nZ code has to work differently to the rest of the code due to the fact that precision doesn't behave the same way. From memory it is possible for precision to increase in this case, which will lead to test failures (hopefully) if you do the same thing as for the other modules.

Eventually we also want to have the code more or less work for inexact fields. That becomes harder with generic code for them all.

@wbhart
Copy link
Contributor

wbhart commented Feb 21, 2021

Actually, Nemo has almost no test code here, so maybe you won't get test failures, just wrong answers, presumably.

@fieker
Copy link
Contributor

fieker commented Feb 24, 2021

powering by p in char p will increase the precision, but then, powerin in char p should be handled differently to start with...

Also bear in mind that the crossover to AA is not clear. In AA we also miss the AbsSeries I think

@wbhart
Copy link
Contributor

wbhart commented Feb 27, 2021

There is AbsSeries in AbstractAlgebra.

As for powering, it doesn't look to increase the precision at present:

julia> R = ResidueField(ZZ, 3)
Residue field of Integer Ring modulo 3

julia> Q, x = PowerSeriesRing(R, 3, "x")
(Univariate power series ring in x over Residue field of Integer Ring modulo 3, x + O(x^4))

julia> (x+x^2)^3
x^3 + O(x^6)

I seem to remember spending ages working on this issue, so I'm not sure what I'm missing. It seems to me the result should be x^3 + O(x^9) here, but both Nemo and AbstractAlgebra return the above at the moment. Perhaps we made the decision to not return the maximum possible precision. I do remember discussing it at length.

@wbhart
Copy link
Contributor

wbhart commented Feb 27, 2021

Regarding powering, see #319

Also note this probably affects multiplication over GF2 and Z/2Z

@wbhart
Copy link
Contributor

wbhart commented Jul 29, 2021

This was done recently. There are now five files for each instead of six, but we also implement two more modules. Further consolidation isn't really worth it.

@wbhart wbhart closed this as completed Jul 29, 2021
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

No branches or pull requests

3 participants