Don't allow grade on graded polynomial ring

[fingolfin]

See PR #5077 for background.

[fingolfin]

I've contemplated allow grade to be used to change the grading of a ring, but this seems like an artificial operation to me -- and one that one can do via grade(forget_grading(R), ...) already.

[fingolfin]

I note that for FreeMod, it seems grade does work on inputs that are already graded -- it seems that it then simply "replaces" the grading. Not sure this is a good idea, and of course if it behaves differently than for rings that's not helpful for users either...

[afkafkafk13]

I support throwing an error and pointing the user to grade(forget_grading(...),...), as this is the clean solution. However, grade is already in use -- as the failing tests show. So this needs to be handled with care.

Pinging @jankoboehm or @wdecker : to make sure that Oscar ends up with a consistent philosophy behind the decisions for rings and modules.

[fingolfin]

The errors so far were in the serialization code, which apparently tests de(serializing) graded graded polynomial rings... I removed one such test, but it seems there is another one triggered from test_1_4_0_upgrade and I don't know which data this references... Maybe @antonydellavecchia can help... Or I'll try to dig into it later this week

[lgoettgens]

The errors so far were in the serialization code, which apparently tests de(serializing) graded graded polynomial rings... I removed one such test, but it seems there is another one triggered from test_1_4_0_upgrade and I don't know which data this references... Maybe @antonydellavecchia can help... Or I'll try to dig into it later this week

The examples for that tests are loaded from the artifact located at

Oscar.jl/Artifacts.toml

Line 41 in 25e6c70

url = "https://github.com/oscar-system/Oscar.jl/releases/download/archive-tag-1/version_1_3_0_files.tar.gz"

The error message tells you that it is the file MPolyDecRing/0.mrdi in the tarball. There is some machinery to exclude some examples from that function (

Oscar.jl/test/Serialization/upgrades/runtests.jl

Line 45 in 25e6c70

"Vector" => 49:97,

), but since that is the only file for MPolyDecRing, it would be good to replace it.

[jankoboehm]

I've contemplated allow grade to be used to change the grading of a ring, but this seems like an artificial operation to me -- and one that one can do via grade(forget_grading(R), ...) already.

We have to throw an error. Degrees of elements can be cached.

[afkafkafk13]

I've contemplated allow grade to be used to change the grading of a ring, but this seems like an artificial operation to me -- and one that one can do via grade(forget_grading(R), ...) already.

We have to throw an error. Degrees of elements can be cached.

Do you already throw an error for grade applied to graded modules? Then everything would be consistent.

[jankoboehm]

I've contemplated allow grade to be used to change the grading of a ring, but this seems like an artificial operation to me -- and one that one can do via grade(forget_grading(R), ...) already.

We have to throw an error. Degrees of elements can be cached.

Do you already throw an error for grade applied to graded modules? Then everything would be consistent.

For free modules the situation is more clear and safe. If you grade a free module you get a new free module with the specified grading. But I think we should always throw an error. I will add the error message.

[fingolfin]

This is stalled and waiting on issue #5100 being resolved.

[fingolfin]

So the file version_1_3_0/MPolyDecRing-42/0.mrdi seems to encode a "doubly wrapped" MPolyDecRing:

{
   "_ns" : {
      "Oscar" : [
         "https://github.com/oscar-system/Oscar.jl",
         "1.3.0-8330fc3d4e91f7d4c131bc5da019cf90a4d760a4"
      ]
   },
   "_refs" : {
      "18943bca-8341-40ac-9aac-04545a09467f" : {
         "_type" : "MPolyDecRing",
         "data" : {
            "grading" : {
               "_type" : {
                  "name" : "Vector",
                  "params" : {
                     "name" : "FinGenAbGroupElem",
                     "params" : "a11d552a-914d-4f7d-a3f8-c974545d1f35"
                  }
               },
               "data" : [
                  [ [ "1", "2" ] ],
                  [ [ "3", "1" ] ]
               ]
            },
            "ring" : "3188c864-627e-4673-9411-c48cb30ede79"
         }
      },
      "3188c864-627e-4673-9411-c48cb30ede79" : {
         "_type" : "MPolyRing",
         "data" : {
            "base_ring" : {
               "_type" : "QQField"
            },
            "symbols" : [ "x", "y" ]
         }
      },
      "a11d552a-914d-4f7d-a3f8-c974545d1f35" : {
         "_type" : "FinGenAbGroup",
         "data" : [
            [ "0", "0" ],
            [ "0", "0" ]
         ]
      },
      "d5b45ca4-166b-402f-96d8-aef989d3092b" : {
         "_type" : "FinGenAbGroup",
         "data" : [
            [ "0", "0" ],
            [ "0", "0" ]
         ]
      }
   },
   "_type" : "MPolyDecRing",
   "data" : {
      "grading" : {
         "_type" : {
            "name" : "Vector",
            "params" : {
               "name" : "FinGenAbGroupElem",
               "params" : "d5b45ca4-166b-402f-96d8-aef989d3092b"
            }
         },
         "data" : [
            [ [ "1", "2" ] ],
            [ [ "3", "1" ] ]
         ]
      },
      "ring" : "18943bca-8341-40ac-9aac-04545a09467f"
   },
   "id" : "a94b9885-fdbc-40e5-8e94-28eee6f799a9"
}

Now I could try to add an update script to deal with this --but it may be non-trivial, as one may have to "merge" two or more rings into one: in the example above, the only "fix" I can think of is to replace any reference to the ring with id "a94b9885-fdbc-40e5-8e94-28eee6f799a9" by a reference to the ring with id "18943bca-8341-40ac-9aac-04545a09467f"`

But does that really make sense? One could create these rings (before this PR) but I think afterwards most non-trivial operations with them and their elements will fail. So one might very well argue this test file contains invalid data.

@antonydellavecchia were exactly did this come from? Maybe from this test case in test/Serialization/PolynomialsSeries.jl ?

    @testset "Graded Ring" begin
      R, (x, y) = QQ[:x, :y]
      A = [1 3; 2 1]
      M, (m1, m2) = grade(R, A)

      test_save_load_roundtrip(path, m1 * m2) do loaded
        @test loaded == m1 * m2
        @test grading_group(parent(loaded)) == grading_group(M)
      end

      GM, _ = grade(M, A)
      test_save_load_roundtrip(path, GM) do loaded
        @test loaded == GM
        @test forget_grading(loaded) == forget_grading(GM)
      end
    end

I believe the second ring GM is a "graded graded ring" and should thus also not be allowed.

[antonydellavecchia]

So the file version_1_3_0/MPolyDecRing-42/0.mrdi seems to encode a "doubly wrapped" MPolyDecRing:

{
   "_ns" : {
      "Oscar" : [
         "https://github.com/oscar-system/Oscar.jl",
         "1.3.0-8330fc3d4e91f7d4c131bc5da019cf90a4d760a4"
      ]
   },
   "_refs" : {
      "18943bca-8341-40ac-9aac-04545a09467f" : {
         "_type" : "MPolyDecRing",
         "data" : {
            "grading" : {
               "_type" : {
                  "name" : "Vector",
                  "params" : {
                     "name" : "FinGenAbGroupElem",
                     "params" : "a11d552a-914d-4f7d-a3f8-c974545d1f35"
                  }
               },
               "data" : [
                  [ [ "1", "2" ] ],
                  [ [ "3", "1" ] ]
               ]
            },
            "ring" : "3188c864-627e-4673-9411-c48cb30ede79"
         }
      },
      "3188c864-627e-4673-9411-c48cb30ede79" : {
         "_type" : "MPolyRing",
         "data" : {
            "base_ring" : {
               "_type" : "QQField"
            },
            "symbols" : [ "x", "y" ]
         }
      },
      "a11d552a-914d-4f7d-a3f8-c974545d1f35" : {
         "_type" : "FinGenAbGroup",
         "data" : [
            [ "0", "0" ],
            [ "0", "0" ]
         ]
      },
      "d5b45ca4-166b-402f-96d8-aef989d3092b" : {
         "_type" : "FinGenAbGroup",
         "data" : [
            [ "0", "0" ],
            [ "0", "0" ]
         ]
      }
   },
   "_type" : "MPolyDecRing",
   "data" : {
      "grading" : {
         "_type" : {
            "name" : "Vector",
            "params" : {
               "name" : "FinGenAbGroupElem",
               "params" : "d5b45ca4-166b-402f-96d8-aef989d3092b"
            }
         },
         "data" : [
            [ [ "1", "2" ] ],
            [ [ "3", "1" ] ]
         ]
      },
      "ring" : "18943bca-8341-40ac-9aac-04545a09467f"
   },
   "id" : "a94b9885-fdbc-40e5-8e94-28eee6f799a9"
}

Now I could try to add an update script to deal with this --but it may be non-trivial, as one may have to "merge" two or more rings into one: in the example above, the only "fix" I can think of is to replace any reference to the ring with id "a94b9885-fdbc-40e5-8e94-28eee6f799a9" by a reference to the ring with id "18943bca-8341-40ac-9aac-04545a09467f"`

But does that really make sense? One could create these rings (before this PR) but I think afterwards most non-trivial operations with them and their elements will fail. So one might very well argue this test file contains invalid data.

@antonydellavecchia were exactly did this come from? Maybe from this test case in test/Serialization/PolynomialsSeries.jl ?

    @testset "Graded Ring" begin
      R, (x, y) = QQ[:x, :y]
      A = [1 3; 2 1]
      M, (m1, m2) = grade(R, A)

      test_save_load_roundtrip(path, m1 * m2) do loaded
        @test loaded == m1 * m2
        @test grading_group(parent(loaded)) == grading_group(M)
      end

      GM, _ = grade(M, A)
      test_save_load_roundtrip(path, GM) do loaded
        @test loaded == GM
        @test forget_grading(loaded) == forget_grading(GM)
      end
    end

I believe the second ring GM is a "graded graded ring" and should thus also not be allowed.

The file certainly came from the test case. Why the test case is there I can't remember.
It might have something to with me trying to add IPC communication to https://github.com/bkholler/MultigradedImplicitization.jl/blob/main/MultigradedImplicitization.jl

I think labeling as invalid data is fair, and I think we can just remove the test file entirely.

We are working on an updated version on MultigradedImplicitization that we want to get into to Oscar
so we'll make sure to adjust accordingly based on this PR.

[fingolfin]

See oscar-system/serialization-upgrade-tests#7

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 84.89%. Comparing base (7f70d84) to head (3f33523).
⚠️ Report is 14 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5080      +/-   ##
==========================================
+ Coverage   84.84%   84.89%   +0.04%     
==========================================
  Files         709      709              
  Lines       95417    96394     +977     
==========================================
+ Hits        80958    81834     +876     
- Misses      14459    14560     +101     

Files with missing lines	Coverage Δ
src/Rings/mpoly-graded.jl	93.37% <100.00%> (+0.01%)	⬆️

... and 23 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[fingolfin]

Thanks @joschmitt @lgoettgens

[jankoboehm]

Fine, but I cannot judge the serialization side of the topic.

[lgoettgens]

The serialization test change is fine