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

Add and use zero!, one!, neg! methods #1869

Merged
merged 4 commits into from
Oct 8, 2024
Merged

Add and use zero!, one!, neg! methods #1869

merged 4 commits into from
Oct 8, 2024

Conversation

fingolfin
Copy link
Member

Add a bunch of one! and neg! methods. Also used zero!, one!, neg! in a few more spots. And placed these function in a uniform way into the code based (i.e. these are now always the first three unsafe operations).

This is incomplete in so far as there are still types missing a "native" neg! (and in some very few cases also one!).

Unfortunately this function is not covered by the ring conformance test suite -- we should change that (in AA).

Copy link

codecov bot commented Sep 27, 2024

Codecov Report

Attention: Patch coverage is 66.00000% with 119 lines in your changes missing coverage. Please review.

Project coverage is 87.37%. Comparing base (ef20920) to head (ab457ad).
Report is 3 commits behind head on master.

Files with missing lines Patch % Lines
src/flint/fmpz_mod.jl 0.00% 8 Missing ⚠️
src/flint/fmpq_mat.jl 40.00% 6 Missing ⚠️
src/flint/fq_default_mat.jl 40.00% 6 Missing ⚠️
src/flint/fq_default_mpoly.jl 0.00% 6 Missing ⚠️
src/flint/fq_mat.jl 40.00% 6 Missing ⚠️
src/flint/fq_nmod_mat.jl 40.00% 6 Missing ⚠️
src/flint/gfp_fmpz_elem.jl 0.00% 6 Missing ⚠️
src/flint/fmpz_mod_rel_series.jl 0.00% 5 Missing ⚠️
src/flint/fq_default_rel_series.jl 58.33% 5 Missing ⚠️
src/flint/fq_rel_series.jl 54.54% 5 Missing ⚠️
... and 18 more
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1869      +/-   ##
==========================================
+ Coverage   87.32%   87.37%   +0.04%     
==========================================
  Files          97       97              
  Lines       35658    36561     +903     
==========================================
+ Hits        31139    31945     +806     
- Misses       4519     4616      +97     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@thofma
Copy link
Member

thofma commented Sep 27, 2024

Seems to be ok. The unfortunate thing is that we would not notice if a method went missing.

@fingolfin fingolfin force-pushed the mh/zero-one-neg branch 2 times, most recently from a8190be to a622e98 Compare September 30, 2024 13:01
Copy link
Collaborator

@lgoettgens lgoettgens left a comment

Choose a reason for hiding this comment

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

found some things. for the valuation issue, can you please check all other relative series types as well (I might have forgotten a comment somewhere).

Overall, I would really like to have Nemocas/AbstractAlgebra.jl#1814 in, but let's merge this once the comments are resolved and fix any issues later

src/flint/fmpq_rel_series.jl Outdated Show resolved Hide resolved
else
z.data = sub!(z.data, R.n, a.data)
end
return z
Copy link
Collaborator

Choose a reason for hiding this comment

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

no idea if this is correct (seems non-trivial enough at least to deserve a comment)

src/flint/fmpz_mod_rel_series.jl Outdated Show resolved Hide resolved
src/flint/fmpz_rel_series.jl Outdated Show resolved Hide resolved
src/flint/fq_default_rel_series.jl Outdated Show resolved Hide resolved
src/flint/fq_rel_series.jl Outdated Show resolved Hide resolved
src/flint/gfp_elem.jl Outdated Show resolved Hide resolved
@fingolfin
Copy link
Member Author

Thanks for the careful feedback, @lgoettgens . I am busy with other things right now, but I plan to go over it all, and maybe use this as incentive to add a few more ring interface tests which hopefully will detect all my mistakes here, to increase confidence in this and further work.

@fingolfin fingolfin marked this pull request as draft October 1, 2024 07:30
@fingolfin fingolfin marked this pull request as ready for review October 1, 2024 23:32
@fingolfin
Copy link
Member Author

I think this is ready in so far as that I addressed review feedback by @lgoettgens and am not aware of other issues right now. But I'd still feel better if we first had the conformance tests in place to properly check all of these. Thankfully @lgoettgens is working on that.

@fingolfin
Copy link
Member Author

I made sure this branch passes the new conformance tests in PR #1880 (I.e. I did run the Nemo tests suite with this branch plus the one in PR #1880 merged, against the code in Nemocas/AbstractAlgebra.jl#1827 and it passed).

As such we could also just merge it now.

src/flint/nmod.jl Outdated Show resolved Hide resolved
@fingolfin
Copy link
Member Author

I've restarted the CI tests to verify the conformance tests pass. They do. I don't see a point in waiting for OscarCI to pass again, as they passed before and nothing changed in the code, just in the tests we run.

@fingolfin fingolfin merged commit 6755a22 into master Oct 8, 2024
47 of 48 checks passed
@fingolfin fingolfin deleted the mh/zero-one-neg branch October 8, 2024 12:09
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