Skip to content

Fix a bug in khuri-makdisi small model #40240

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

Merged
merged 4 commits into from
Jun 14, 2025

Conversation

kwankyu
Copy link
Collaborator

@kwankyu kwankyu commented Jun 10, 2025

Fixes #40237.

📝 Checklist

  • The title is concise and informative.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation and checked the documentation preview.

⌛ Dependencies

@kwankyu kwankyu force-pushed the p/fix-bug-small-khuri-makdisi branch from 8344016 to af7d348 Compare June 10, 2025 07:01
Copy link

github-actions bot commented Jun 10, 2025

Documentation preview for this PR (built with commit 2d2592d; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

@kwankyu kwankyu marked this pull request as ready for review June 10, 2025 08:17
Copy link
Member

@vincentmacri vincentmacri left a comment

Choose a reason for hiding this comment

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

Thanks for the quick fix!

Requesting two small changes to the doctest.

I think I need a bit of help/context to understand what's going on here before I can approve this (I've only glanced at Khuri-Makdisi's paper so I'm not very familiar with how the algorithm works). Can you explain a bit more (just in GitHub, you don't need to add the explanation to the code unless you think it's necessary) about what's going on at this part of the code/what was causing this bug and why this (correctly) fixes it?

@kwankyu
Copy link
Collaborator Author

kwankyu commented Jun 10, 2025

As the added comment says

# The row space of w2 represents H^0(O(3D_0 - D1 - D2)), whose
# dimension is at least d0 - g + 1. Hence the codimension is at most
# 2*d0, and we cannot provide an expected_codim argument for mu_preimage.
w2 = self.mu_preimage(self.wV3, w1, self.mu_mat33)

we cannot guarantee the dimension of $H^0(O(3D_0 - D1 - D2))$ is exactly $d_0 - g +1$, according to the Riemann-Roch theorem, since the degree of the divisor can be as small as $g+1$ for the small model (the divisor is special). Hence $2d_0$ is not the expected codimension; $2d_0$ is only an upper bound for the codimension. By the way, expected_codim is used to speed up mu_preimage when the codimension is known.

@vincentmacri
Copy link
Member

Alright, looks good to me then.

I know that you know this because you reviewed #40221, but for anyone else reading, the long test warnings in the new doctest are fixed by #40221 which has been approved and is waiting to be merged into develop.

@kwankyu
Copy link
Collaborator Author

kwankyu commented Jun 11, 2025

Thanks!

@vbraun vbraun merged commit 63b5554 into sagemath:develop Jun 14, 2025
26 of 27 checks passed
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.

Assertion error in certain Khuri-Makdisi small arithmetic operations
3 participants