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.

Sign up for GitHub

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

Jump to bottom

Some fixes & improvements to autogenerated flint .pxd #37193

Merged
merged 5 commits into from
Feb 13, 2024
Merged

Some fixes & improvements to autogenerated flint .pxd #37193

merged 5 commits into from
Feb 13, 2024

Conversation

mezzarobba
Copy link
Member

  • flint autogen: fields of a[rc]b_poly_struct
  • flint autogen: remove two typedefs that don't exist in flint
  • move fix from Fix build when linbox is build with fplll support #37064 to the flint interface generator
  • flint autogen: add warnings about generated files to the templates
  • regenerate flint wrappers

@mezzarobba mezzarobba requested a review from videlec January 30, 2024 13:47
@videlec
Copy link
Contributor

videlec commented Jan 30, 2024

Interesting

sage -t --long --random-seed=286735480429121101562228604801325644303 sage/rings/tests.py
**********************************************************************
File "sage/rings/tests.py", line 453, in sage.rings.tests.?
Failed example:
    test_karatsuba_multiplication(QQbar, 3, 3, numtests=2)    # long time, needs sage.rings.number_field
Expected nothing
Got:
    Random testing has revealed a problem in test_karatsuba_multiplication
    Please report this bug!  You may be the first
    person in the world to have seen this problem.
    Please include this random seed in your bug report:
    Random seed: 327619870453015189736949675324415167773
    ValueError('Multiplication failed')
**********************************************************************
1 item had failures:
   1 of  13 in sage.rings.tests.?
    [62 tests, 1 failure, 1191.47 s]

@tornaria
Copy link
Contributor

The autogen scripts seem to be run manually, and the output files are committed to source. Thus, it doesn't make sense to ship these scripts with sage_setup, does it?

@mezzarobba
Copy link
Member Author

Interesting

sage -t --long --random-seed=286735480429121101562228604801325644303 sage/rings/tests.py
**********************************************************************
File "sage/rings/tests.py", line 453, in sage.rings.tests.?
Failed example:
    test_karatsuba_multiplication(QQbar, 3, 3, numtests=2)    # long time, needs sage.rings.number_field
Expected nothing
Got:
    Random testing has revealed a problem in test_karatsuba_multiplication
    Please report this bug!  You may be the first
    person in the world to have seen this problem.
    Please include this random seed in your bug report:
    Random seed: 327619870453015189736949675324415167773
    ValueError('Multiplication failed')
**********************************************************************
1 item had failures:
   1 of  13 in sage.rings.tests.?
    [62 tests, 1 failure, 1191.47 s]

Can't reproduce locally.

@mezzarobba
Copy link
Member Author

The autogen scripts seem to be run manually, and the output files are committed to source.

Yes.

Thus, it doesn't make sense to ship these scripts with sage_setup, does it?

I think I agree.

@mezzarobba
flint autogen: fields of a[rc]b_poly_struct
d823c43 
Rationale: accessing the coefficients of degree bewteen length and alloc
of a polynomial is done via pol->coeffs, there is (afaik) no macro for
that. And these fields are documented.
@mezzarobba
flint autogen: remove two typedefs that don't exist in flint
956ab2c
@mezzarobba
move fix from sagemath#37064 to the flint interface generator
98c3871
@mezzarobba
flint autogen: add warnings about generated files to the templates
c0ba507 
(it didn't seem worth the trouble modifying the generator itself so that
it inserts the warning in the correct place...)
@mezzarobba
regenerate flint wrappers
c179886 
using flint commit 3e2c3a3e091106a25ca9c6fba28e02f2cbcd654a, which is a
bit more recent than the flint version sage officially depends on but
matches the API currently exposed in the generated files
@mezzarobba mezzarobba force-pushed the flint-autogen-tweaks branch from bd29c6c to c179886 Compare February 3, 2024 12:53
@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 3, 2024

Documentation preview for this PR (built with commit c179886; changes) is ready! 🎉

@mezzarobba mezzarobba mentioned this pull request Feb 4, 2024
Closed
@mezzarobba
Copy link
Member Author

The two test failures are now #37216 and #37238 and don't seem to have anything to do with this PR. @videlec, thank you for reviewing the changes; can I interpret the gh approval as positive review?

vbraun pushed a commit to vbraun/sage that referenced this pull request Feb 7, 2024
a02ae74 
sagemathgh-37193: Some fixes & improvements to autogenerated flint .pxd
    
- flint autogen: fields of a[rc]b_poly_struct
- flint autogen: remove two typedefs that don't exist in flint
- move fix from sagemath#37064 to the flint interface generator
- flint autogen: add warnings about generated files to the templates
- regenerate flint wrappers
    
URL: sagemath#37193
Reported by: Marc Mezzarobba
Reviewer(s): Vincent Delecroix
@vbraun vbraun merged commit adee77d into sagemath:develop Feb 13, 2024
19 of 20 checks passed
@mkoeppe mkoeppe added this to the sage-10.3 milestone Mar 7, 2024
@mezzarobba mezzarobba mentioned this pull request Mar 7, 2024
Open
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@videlec videlec videlec approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
sage-10.3
Development

Successfully merging this pull request may close these issues.
5 participants
@mezzarobba @videlec @tornaria @vbraun @mkoeppe