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

Improve mpoly context interface #225

Merged
merged 11 commits into from
Sep 15, 2024
Merged

Improve mpoly context interface #225

merged 11 commits into from
Sep 15, 2024

Conversation

Jake-Moss
Copy link
Contributor

Relevant issue: #195

This PR:

  • Renames get_context -> get.
  • Disables the __init__ method for mpolys. If you really need a new context use _new_ on a concrete subclass. Otherwise use get.
  • Removes the number of variables arg from the get method. Only the names iterable, ordering, and when appropriate modulus is required now.
  • Improves the variable naming system, e.g.
              >>> flint_mpoly_context.create_variable_names('x')
              ('x',)
              >>> flint_mpoly_context.create_variable_names(('x', 3))
              ('x0', 'x1', 'x2')
              >>> flint_mpoly_context.create_variable_names([('x', 3), 'y'])
              ('x0', 'x1', 'x2', 'y')
    This applies to the get method indirectly
  • Allows ordering to be specified as a string. The ordering enum is now also a enum.StrEnum so it should work seamlessly.
  • Hides the any_as_scalar and scalar_as_mpoly methods. These should never have been public facing and are inconsistent at the moment.
    The nmod_mpoly.any_as_scalar method is the only method to actually use self, this is required to get the modulus to allow conversion from arbitrary ints that are larger than ulong can hold
    I've also removed the exceptions they raised, they never did anything anyway
  • Adds an intermediate flint_mod_mpoly_context class to hold some shared methods for the mod mpoly types.

@Jake-Moss
Copy link
Contributor Author

Ah whoops, didn't realise enum.StrEnum was added in 3.11
https://docs.python.org/3/library/enum.html#enum.StrEnum

@oscarbenjamin
Copy link
Collaborator

Looks good. Thanks!

@oscarbenjamin oscarbenjamin merged commit ae4fe25 into flintlib:main Sep 15, 2024
40 checks passed
@@ -265,22 +264,63 @@ cdef class flint_poly(flint_elem):
raise NotImplementedError("Complex roots are not supported for this polynomial")


class Ordering(enum.Enum):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not an urgent issue but defining this here and not in the .pxd means that the type cannot be imported with cimport and used as the type declaration on cdef functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure how that would be achieved, though I might just be missing something.
A definition like

class Ordering(enum.Enum):
    lex = "lex"
    deglex = "deglex"
    degrevlex = "degrevlex"

cannot go in the pxd file. Similarly neither can an empty definition like

class Ordering(enum.Enum):
    pass

The class can't be declared as a cdef class because enum.Enum is not an extension type, which is also why the ordering_py_to_c and ordering_c_to_py functions are not members. This was why it was a cython enum previously.

Cython enums don't support string values, and it complains that a byte literal with more than one char isn't coercible to an integer, fair enough, C enums are only ints anyway. With a single byte the Cython compiler crashes.

We could go back to using a Cython cpdef enum with int values, but then using it within a type hint raises a very unhelpful error message when passed something that's not an int, "TypeError: an integer is required". The range of the enum isn't even enforced.

# in a .pyx
cpdef enum CheeseState:
    hard = 1
    soft = 2
    runny = 3

def cheese(arg: CheeseState):
    print(arg)
>>> cheese(1)
1
>>> cheese(100000)
100000
>>> cheese("test")
...
TypeError: an integer is required

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could ofc just do it ourselves with string values and functions to go between

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, I see it is a subclass of a Python class.

The whole enum business seems unnecessarily confusing in Python and Cython.

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