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

Bump version to 0.5.0 #84

Merged
merged 9 commits into from
Oct 22, 2023
Merged

Bump version to 0.5.0 #84

merged 9 commits into from
Oct 22, 2023

Conversation

GiacomoPope
Copy link
Contributor

Version bump to show new type added. Removed a TODO from the README and another message in the change log to discuss the integer roots from #72

@GiacomoPope GiacomoPope marked this pull request as ready for review September 16, 2023 09:52
@GiacomoPope
Copy link
Contributor Author

Potentially we want to fix #74 before updating the version, but I don't have a good idea how to fix this myself.

README.md Outdated
Comment on lines 130 to 131
- gh-63: The `roots` method of `arb_poly`, and `nmod_poly` is no longer supported. Use `acb_roots(p).roots()` to get the old behaviour of returning the roots as `acb`. Note that the `roots` method of `fmpz_poly` and `fmpq_poly` currently returns the complex roots of the polynomial.
- gh-72: The roots method of `arb_poly` is not supported. Use either the `complex_roots` method or `acb_roots(p).roots()` to get the old behaviour of returning the complex roots. The `roots` method on `fmpz_poly` and `fmpq_poly` now return integer and rational roots respectively. To access complex roots on these types, use the `complex_roots` method. For `acb_poly`, both `roots` and `complex_roots` behave the same.
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should also add notes for other PRs here.

Big refactor across several versions:

For 0.4.4 there should be

For 0.4.3

We should get better at adding release notes here. It is important for people to see these so that they can see that development is happening. Also I think that each note should list the names of the authors. It is informative for anyone who wants to understand the project to see that there are multiple people working on things.

Copy link
Collaborator

Choose a reason for hiding this comment

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

#63 was for 0.4.3 not 0.5.0

For 0.4.2

For 0.4.1

For 0.4.0

Copy link
Collaborator

Choose a reason for hiding this comment

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

For 0.4.3 #74 should be noted.

@oscarbenjamin
Copy link
Collaborator

The Windows tests failed:

test_fmpz_mod...Flint exception (General error):
    mkstemp failed

That is because of #80. Note that my original PR for #80 noted this: #43 (comment) although somehow the tests passed.

It is good that we spotted this before release: we can't use flint 3-alpha1 because of the Windows Flint bug that was fixed in flintlib/flint#1416

@oscarbenjamin
Copy link
Collaborator

So we either revert #80 or wait for a new (pre-)release of Flint 3 before making a new release of python-flint.

@GiacomoPope
Copy link
Contributor Author

Do we have a timeline on the next pre-release? I guess that determines the best thing to do. I will find time today to address the comments above and do a more detailed change log for all these PR.

For "authors" how would you like to do it? I'm happy with whatever

@oscarbenjamin
Copy link
Collaborator

For "authors" how would you like to do it? I'm happy with whatever

I don't know really. Maybe just linking to the PRs is fine.

@GiacomoPope
Copy link
Contributor Author

For progress, I want to try and do fmpz_mod_poly, maybe if I finish this and the 3.0 bug remains we can revert #80 and if it's fixed by then it's all OK? I doubt the small addition for fmpz_mod will really help anyone right now.

@oscarbenjamin
Copy link
Collaborator

We can consider reverting #80 at some point if it takes a while for a new release of Flint. for now I don't think it is urgent.

Especially if we expect that many new features might be added in a relatively short time there is no rush to get a release out for each one.

@oscarbenjamin
Copy link
Collaborator

Rerun CI after gh-94 was merged

@oscarbenjamin
Copy link
Collaborator

Some new Windows problem:

test_fmpz_mod...OK
test_fmpz_mod_dlog...
Error: Process completed with exit code 1.

Further investigation needed...

@oscarbenjamin
Copy link
Collaborator

Twice now this pull request has picked up Windows problems that do not seem to show up elsewhere in CI. The changes here should not affect anything significant so I don't understand why that happens.

@GiacomoPope
Copy link
Contributor Author

GiacomoPope commented Oct 14, 2023 via email

@oscarbenjamin
Copy link
Collaborator

I can reproduce the problem on Windows locally with master so it is not this PR that causes the problem but there is a bug that causes a crash on Windows.

It is always the discrete log test that crashes and specifically it crashes here:

for _ in range(10):
g = F(random.randint(0,p))
for _ in range(10):
i = random.randint(0,p)
a = g**i
x = g.discrete_log(a)
assert g**x == a

The crash happens in x = g.discrete_log(a) for the 1st iteration of the inner loop on the 2nd or 3rd iteration of the outer loop.

I think that this is most likely a memory bug in python-flint. The memory management for fmpz_mod and fmpz_mod_ctx is suspicious:

cdef class fmpz_mod_ctx:
cdef fmpz_mod_ctx_t val
cdef bint _is_prime
cdef fmpz_mod_discrete_log_pohlig_hellman_t *L
cdef set_any_as_fmpz_mod(self, fmpz_t val, obj)
cdef any_as_fmpz_mod(self, obj)
cdef _precompute_dlog_prime(self)
cdef class fmpz_mod(flint_scalar):
cdef fmpz_mod_ctx ctx
cdef fmpz_t val
cdef fmpz_t *x_g

I think this is something to do with either L or x_g which will be initialised on a 1st call to g.discrete_log(a) for a given g.

@oscarbenjamin
Copy link
Collaborator

Here when an fmpz_mod is created x_g is set to NULL and when it is deallocated a test if self.x_g is used:

def __cinit__(self):
fmpz_init(self.val)
self.x_g = NULL
def __dealloc__(self):
fmpz_clear(self.val)
if self.x_g:
fmpz_clear(self.x_g[0])
libc.stdlib.free(self.x_g)

I think firstly that the test should be if self.x_g is NULL although it is not clear if that is the problem.

A call to discrete_log will initialise this:

if not self.ctx.L:
self.ctx._precompute_dlog_prime()
# Solve the discrete log for the chosen base and target
# g = y^x_g and a = y^x_a
# We want to find x such that a = g^x =>
# (y^x_a) = (y^x_g)^x => x = (x_a / x_g) mod (p-1)
# For repeated calls to discrete_log, it's more efficient to
# store x_g rather than keep computing it
if not self.x_g:
self.x_g = <fmpz_t *>libc.stdlib.malloc(
cython.sizeof(fmpz_t)
)
fmpz_mod_discrete_log_pohlig_hellman_run(
self.x_g[0], self.ctx.L[0], self.val
)

Again I think is NULL should be tested.

We also have:

cdef _precompute_dlog_prime(self):
"""
Initalise the dlog data, all discrete logs are solved with an
internally chosen base `y`
"""
self.L = <fmpz_mod_discrete_log_pohlig_hellman_t *>libc.stdlib.malloc(
cython.sizeof(fmpz_mod_discrete_log_pohlig_hellman_struct)
)
fmpz_mod_discrete_log_pohlig_hellman_init(self.L[0])
fmpz_mod_discrete_log_pohlig_hellman_precompute_prime(
self.L[0], self.val.n
)

I think firstly that it would be better to avoid the use of malloc/free wherever possible in favour of using CPython-style memory management.

@oscarbenjamin
Copy link
Collaborator

This all seems to be working with flint 3.0.0-rc1 now after gh-95.

I've tested locally on Windows that everything seems to be working.

@oscarbenjamin
Copy link
Collaborator

Rerun after #98 with Flint 3.0.0 final.

@oscarbenjamin
Copy link
Collaborator

All tests have passed. Previously this PR strangely seemed to flag up errors but I don't see any now.

I think that fmpz_mod_poly should be added to the changelog but otherwise this is good for a new release of python-flint.

@oscarbenjamin oscarbenjamin changed the title Bump version to 0.4.5 Bump version to 0.5.0 Oct 21, 2023
@GiacomoPope
Copy link
Contributor Author

I think that fmpz_mod_poly should be added to the changelog but otherwise this is good for a new release of python-flint.

I went to do this just now, but looks like you beat me to it!

@oscarbenjamin
Copy link
Collaborator

It is expected that the build from PyPI job will fail because it is trying to build in an environment that has Flint 3 and does not have Arb (as a separate library). Until the PyPI sdist is updated that job should fail with the changes here. Previously it was passing because it was installing the binary wheel but I have just added --no-binary to force it to build properly.

@oscarbenjamin
Copy link
Collaborator

Okay I think this is ready for a 0.5.0 release.

@GiacomoPope
Copy link
Contributor Author

Yeah I agree.

@oscarbenjamin
Copy link
Collaborator

Okay, let's do it!

@oscarbenjamin oscarbenjamin merged commit 9252d17 into flintlib:master Oct 22, 2023
19 of 21 checks passed
@oscarbenjamin
Copy link
Collaborator

@fredrik-johansson the docs for python-flint have not been updated sine 0.3.0 (5 years ago).

As of 0.5.0 there are some significant new sections in the docs for fmpz_mod and fmpz_mod_poly as well as changes to the build instructions (although those might actually need updating again).

Ideally docs updates would be handled automatically in CI along with pushing to PyPI but for now at least I am pushing manually to PyPI.

The docs are hosted on your website fredrikj.net though so I don't know if you would prefer to push the updates there yourself manually. Alternatively we could designate a repo somewhere for hosting the built docs that could be pulled automatically by fredrikj.net or something.

@fredrik-johansson
Copy link
Collaborator

Maybe host the docs on readthedocs or somesuch? We could do something automatic on my server but I'm not too eager to set that up right now.

@oscarbenjamin
Copy link
Collaborator

I'll have a look at setting up read the docs.

I have finally managed to upload the release files to PyPI:

https://pypi.org/project/python-flint/#files

It took 3 hours with a lot restarting twine to get those files up there. Maybe there is a better way to do the uploading.

I need to go do other things now but I'll put out announcements later.

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.

3 participants