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

proto type for many to one type serialization #4331

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

antonydellavecchia
Copy link
Collaborator

@antonydellavecchia antonydellavecchia commented Nov 19, 2024

This pull request should not neccesarily be merged.

It is just to demonstrate a possible mechanic for having a many types to one serialization
where multiple types share the same encoding.

I put it in a pull request to highlight the changes and to start to discuss if this is viable solution and
to discuss the potential pitfalls of such a mechanism.

I have an unsafe line of code that I comment on, but this should be fixable, I just couldn't figure out
why sometimes string(fpField) = "Nemo.fpField" and sometimes it is string(fpField) = "fpField"

@antonydellavecchia
Copy link
Collaborator Author

antonydellavecchia commented Nov 20, 2024

Default type storing FqField only the type name has changed.

{
  "_ns": {
    "Oscar": [
      "https://github.com/oscar-system/Oscar.jl",
      "1.3.0-DEV-2ebe51fbb819d32ee59f8f43cd3fb506b796b249"
    ]
  },
  "_type": "FiniteField",
  "data": "2",
  "id": "aad30397-63b7-4082-a55f-4cc425217b83"
}

Non default types have _instance keyword which is a string of the Oscar type that it was before serialization.

{
  "_instance": "fpField",
  "_ns": {
    "Oscar": [
      "https://github.com/oscar-system/Oscar.jl",
      "1.3.0-DEV-2ebe51fbb819d32ee59f8f43cd3fb506b796b249"
    ]
  },
  "_type": "FiniteField",
  "data": "2"
}

@micjoswig
Copy link
Member

Should I be irritated by these comments in the code?

if haskey(s.obj, :_instance)
   # to be safe we need this check but I can't figure out how to get sending
   # types to strings to behave properly
   #s.obj["_instance"] in keys(reverse_type_map[s.obj[type_key]])
   T = eval(Meta.parse(s.obj["_instance"]))
 else
   T = decode_type(s)
 end

What does it mean that "I can't figure out how to get sending types to strings to behave properly"?

Copy link
Member

@micjoswig micjoswig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above.

@antonydellavecchia
Copy link
Collaborator Author

antonydellavecchia commented Nov 20, 2024

Should I be irritated by these comments in the code?

if haskey(s.obj, :_instance)
   # to be safe we need this check but I can't figure out how to get sending
   # types to strings to behave properly
   #s.obj["_instance"] in keys(reverse_type_map[s.obj[type_key]])
   T = eval(Meta.parse(s.obj["_instance"]))
 else
   T = decode_type(s)
 end

What does it mean that "I can't figure out how to get sending types to strings to behave properly"?

This is mainly a todo for me, I describe this issue in the description for the pull request. It appears that string(fpField) = "Nemo.fpField" and sometimes it is string(fpField) = "fpField". I do believe this is fixable, I just haven't had the time to figure out how yet.

I've commented in the code the places where the unexpected results are happening (see following commit)

@antonydellavecchia
Copy link
Collaborator Author

The changes would have the following consequences on the example in the paper.
With a bit of work it should also be possible to not have the "_instance" key appear in the types such as PolyRing

{
  "_ns": {
    "Oscar": [
      "https://github.com/oscar-system/Oscar.jl",
      "1.3.0-DEV-4d1a3ffa24b8e340470afa6b40c02150e3558b4e"
    ]
  },
  "_refs": {
    "0377c284-0b31-4d83-b009-0776e883e80b": {
      "_instance": "FqMPolyRing",
      "_type": "MPolyRing",
      "data": {
        "base_ring": "8c4b9843-a0e1-4306-aff3-1a8dca4954a0",
        "symbols": [
          "y",
          "z"
        ]
      }
    },
    "8c4b9843-a0e1-4306-aff3-1a8dca4954a0": {
      "_type": "FiniteField",
      "data": {
        "def_pol": {
          "_type": {
            "name": "PolyRingElem",
            "params": "c22521f2-ddaf-4125-9c34-c5294f59602d"
          },
          "data": [
            [
              "0",
              "3"
            ],
            [
              "1",
              "6"
            ],
            [
              "2",
              "1"
            ]
          ]
        }
      }
    },
    "c22521f2-ddaf-4125-9c34-c5294f59602d": {
      "_instance": "FqPolyRing",
      "_type": "PolyRing",
      "data": {
        "base_ring": "fd0fa33d-250c-46e6-8d11-0ff6969625bd",
        "symbols": [
          "x"
        ]
      }
    },
    "fd0fa33d-250c-46e6-8d11-0ff6969625bd": {
      "_type": "FiniteField",
      "data": "7"
    }
  },
  "_type": {
    "name": "MPolyRingElem",
    "params": "0377c284-0b31-4d83-b009-0776e883e80b"
  },
  "data": [
    [
      [
        "3",
        "4"
      ],
      [
        [
          "0",
          "2"
        ]
      ]
    ],
    [
      [
        "1",
        "0"
      ],
      [
        [
          "1",
          "5"
        ]
      ]
    ],
    [
      [
        "0",
        "2"
      ],
      [
        [
          "0",
          "3"
        ],
        [
          "1",
          "1"
        ]
      ]
    ],
    [
      [
        "0",
        "0"
      ],
      [
        [
          "0",
          "1"
        ]
      ]
    ]
  ]
}

@antonydellavecchia
Copy link
Collaborator Author

with the latest commit example file looks like this

{
  "_ns": {
    "Oscar": [
      "https://github.com/oscar-system/Oscar.jl",
      "1.3.0-DEV-4d1a3ffa24b8e340470afa6b40c02150e3558b4e"
    ]
  },
  "_refs": {
    "2f5028cc-0fd6-4381-8f7e-0ff1520da04d": {
      "_instance": "FqPolyRing",
      "_type": "PolyRing",
      "data": {
        "base_ring": "d58dcad8-17fa-48aa-aaf9-3a3cd3f86fd4",
        "symbols": [
          "x"
        ]
      }
    },
    "5c637058-63cd-4773-84f1-965164c98ab5": {
      "_instance": "FqField",
      "_type": "FiniteField",
      "data": {
        "def_pol": {
          "_type": {
            "name": "PolyRingElem",
            "params": "2f5028cc-0fd6-4381-8f7e-0ff1520da04d"
          },
          "data": [
            [
              "0",
              "3"
            ],
            [
              "1",
              "6"
            ],
            [
              "2",
              "1"
            ]
          ]
        }
      }
    },
    "d58dcad8-17fa-48aa-aaf9-3a3cd3f86fd4": {
      "_instance": "FqField",
      "_type": "FiniteField",
      "data": "7"
    },
    "dc9554a8-0ed5-4504-9c21-7760d6c4d02c": {
      "_instance": "FqMPolyRing",
      "_type": "MPolyRing",
      "data": {
        "base_ring": "5c637058-63cd-4773-84f1-965164c98ab5",
        "symbols": [
          "y",
          "z"
        ]
      }
    }
  },
  "_type": {
    "name": "MPolyRingElem",
    "params": "dc9554a8-0ed5-4504-9c21-7760d6c4d02c"
  },
  "data": [
    [
      [
        "3",
        "4"
      ],
      [
        [
          "0",
          "2"
        ]
      ]
    ],
    [
      [
        "1",
        "0"
      ],
      [
        [
          "1",
          "5"
        ]
      ]
    ],
    [
      [
        "0",
        "2"
      ],
      [
        [
          "0",
          "3"
        ],
        [
          "1",
          "1"
        ]
      ]
    ],
    [
      [
        "0",
        "0"
      ],
      [
        [
          "0",
          "1"
        ]
      ]
    ]
  ]
}

@micjoswig
Copy link
Member

We agreed that there should be an _instance entry every time where the many to one map is not bijective, even if suitable OSCAR defaults are in place.

With this addition the PR is good for merge IMHO.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants