FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Improve mpoly context interface by Jake-Moss · Pull Request #225 · flintlib/python-flint · GitHub

  1. flintlib
  2. python-flint

Repository navigation

Improve mpoly context interface - #225

Merged
oscarbenjamin merged 11 commits into
flintlib:mainfrom
Jake-Moss:main
Sep 15, 2024
ConversationCommits (11)ChecksFiles changed
Merged

Improve mpoly context interface#225
oscarbenjamin merged 11 commits into
flintlib:mainfrom
Jake-Moss:main

Conversation

Jake-Moss commented Sep 15, 2024

Copy link
Copy Markdown
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 commented Sep 15, 2024

Copy link
Copy Markdown
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 commented Sep 15, 2024

Copy link
Copy Markdown
Collaborator

Looks good. Thanks!

oscarbenjamin merged commit ae4fe25 into flintlib:main Sep 15, 2024
oscarbenjamin mentioned this pull request Sep 15, 2024
Closed
oscarbenjamin reviewed Sep 15, 2024
View reviewed changes
Comment thread src/flint/flint_base/flint_base.pyx
raise NotImplementedError("Complex roots are not supported for this polynomial")


class Ordering(enum.Enum):

oscarbenjamin Sep 15, 2024

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Jake-Moss Sep 15, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Jake-Moss Sep 15, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

oscarbenjamin Sep 15, 2024

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

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

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

oscarbenjamin oscarbenjamin left review comments

Assignees

No one assigned

Labels

type: new feature

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

Back | FazBrowse Home | New Git URL