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

Add fmpq_mpoly.zpoly and fmpq_mpoly.zcontent (fixes #419) by magv · Pull Request #421 · flintlib/python-flint · GitHub

Repository navigation

Add fmpq_mpoly.zpoly and fmpq_mpoly.zcontent (fixes #419) - #421

Merged
oscarbenjamin merged 7 commits into
flintlib:mainfrom
magv:fmpq_mpoly_zpoly_ref
Aug 26, 2026
Merged

oscarbenjamin merged 7 commits into
flintlib:mainfrom
magv:fmpq_mpoly_zpoly_ref

Conversation

magv commented Aug 26, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

This fixes issue #419.

magv force-pushed the fmpq_mpoly_zpoly_ref branch from 13d99ae to 43dc15d Compare August 26, 2026 13:28

magv commented Aug 26, 2026

Copy link
Copy Markdown
Contributor Author

Sorry for the force-push, I've added docstring tests to both functions.

def term_content(self) -> fmpq_mpoly: ...

def zpoly(self) -> fmpz_mpoly: ...
def zcontent(self) -> fmpq: ...

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

I guess zcontent isn't a good name for this if it is fmpq.

Comment thread README.md Outdated
Comment on lines +171 to +178
- Insert Name (IN)

Changes (0.10.0):

- [gh-322](https://github.com/flintlib/python-flint/pull/322), Add `zcontent`
and `zpoly` methods to `fmpq_mpoly`, to get the primitive integer polynomial
of self and the `fmpq` content (the internal representation of the
`fmpq_mpoly`). (IN)

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

Can you add your name and initial to the release note section I just added?

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

Actually the PR number should be 421...

Copy link
Copy Markdown
Collaborator

Maybe it would be better to add content and primitive methods like with fmpz_mpoly. The difference would just be the sign when the zcontent is negative since content for fmpz_poly/fmpz_mpoly is nonnegative gcd. That would sometimes require negating the zpoly but presumably that is not much more expensive than making a copy anyway.

Compare to fmpz_mpoly the difference would also be that the primitive part is of a different type (fmpq_mpoly -> fmpz_mpoly) but maybe that is just to be expected here...

magv commented Aug 26, 2026

Copy link
Copy Markdown
Contributor Author

What about content and zprimitive then? :)

Copy link
Copy Markdown
Collaborator

This version of content is not equivalent because of the sign:

In [13]: from flint import *

In [14]: Rz = fmpz_mpoly_ctx.get(['x', 'y'])

In [15]: Rq = fmpq_mpoly_ctx.get(['x', 'y'])

In [16]: xz, yz = Rz.gens()

In [17]: xq, yq = Rq.gens()

In [18]: (-xz).content()
Out[18]: 1

In [19]: (-xq).content()
Out[19]: -1

I think it would be good to have primitive and content with them being consistent with the same methods for fmpz_mpoly. Then primitive should actually return (fmpq, fmpq_mpoly) to stay in the ring.

So maybe it still makes sense to have separate methods for this kind of content that just gets the internal representation. Maybe zcontent and zpoly are fine. At least the names show that they go together.

magv force-pushed the fmpq_mpoly_zpoly_ref branch from 40862be to 7964fe4 Compare August 26, 2026 16:01
Also fix their docstring tests.
magv force-pushed the fmpq_mpoly_zpoly_ref branch from 7964fe4 to 1f3c4a5 Compare August 26, 2026 16:04

magv commented Aug 26, 2026

Copy link
Copy Markdown
Contributor Author

I've renamed it. The naming inconsistency really comes from the underlying FLINT API. As long as python-flint stays a relatively thin wrapper, it will show up one way or another.

Copy link
Copy Markdown
Collaborator

FLINT has these two functions:

  fmpq_mpoly_content_ref
  fmpq_mpoly_content

The first is for accessing the struct member and the second is for computing the mathematical content. They can differ by a minus sign. I agree that is a confusing naming convention but that's why it is important to consider names of things in the first place.

I think that zcontent and zprimitive are fine as long as the names are clearly distinct from .content() and .primitive() as used elsewhere.

The CI job would sporadically fail because FLINT 3.1 has a bug that
makes it fail to build on newer GitHub Actions runners (with e.g. znver3
as the CPU name).

Copy link
Copy Markdown
Collaborator

I've renamed it from zprimitive back to zpoly. I think that zpoly matches the struct method name so someone familiar with the FLINT struct can find it. Maybe zcontent is misleading but the two method names match so it is clear that they go together. I think that's the best we can do here.

Copy link
Copy Markdown
Collaborator

Okay, looks good. Thanks

oscarbenjamin merged commit 6708168 into flintlib:main Aug 26, 2026
105 checks passed
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
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

Successfully merging this pull request may close these issues.

2 participants


Back | FazBrowse Home | New Git URL