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 is_zero_entry method; remove is_zero_row methods #1801

Merged
merged 1 commit into from
Jun 25, 2024

Conversation

fingolfin
Copy link
Member

With the addition of the new is_zero_entry method, the removed
is_zero_row methods are not faster than the generic one. Also add some
tests, which were also used for benchmarking, together with

@benchmark Nemo.is_zero_row($M,4)

Copy link

codecov bot commented Jun 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.96%. Comparing base (784c28a) to head (4fc5241).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1801   +/-   ##
=======================================
  Coverage   85.95%   85.96%           
=======================================
  Files          95       95           
  Lines       36417    36398   -19     
=======================================
- Hits        31303    31290   -13     
+ Misses       5114     5108    -6     

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

With the addition of the new is_zero_entry method, the removed
is_zero_row methods are not faster than the generic one. Also add some
tests, which were also used for benchmarking, together with

    @benchmark Nemo.is_zero_row($M,4)
@fingolfin fingolfin changed the title Add a is_zero_entry method; remove is_zero_row methods Add is_zero_entry method; remove is_zero_row methods Jun 24, 2024
@lgoettgens
Copy link
Collaborator

Can someone (@fingolfin @thofma) un-require the nightly CI job until oscar-system/Oscar.jl#3882 is resolved?

@fingolfin
Copy link
Member Author

Huh, from my POV the tests against Julia nightly should never be required. Changed it.

@lgoettgens lgoettgens merged commit 5ec098c into Nemocas:master Jun 25, 2024
24 of 26 checks passed
@fingolfin fingolfin deleted the mh/is_zero_row branch June 25, 2024 17:49
@fieker
Copy link
Contributor

fieker commented Jun 26, 2024

is_zero_row should be done for ZZ and ZZ mod, but in a useful way. The removed implementation is highly non-optimal, this can be sped up by a factor of 10 or so

@fieker
Copy link
Contributor

fieker commented Jun 26, 2024

although it is probably non critical, timewise

@lgoettgens
Copy link
Collaborator

There is actually a generic function in https://github.com/Nemocas/AbstractAlgebra.jl/blob/762eab1920547d2e4c3f3e77a4976fff119bf206/src/Matrix.jl#L247-L254, that delegates to a Nemo-backed is_zero_entry. Thus Max's removal is essentially no change at all.

@fieker
Copy link
Contributor

fieker commented Jun 26, 2024

the point is that is_zero_row for ZZ should be done via

  • get the pointer of the row (no c-call!)
  • check if the entries are binary zero (no c-call)
    Currently it uses 2 c-calls per entry hence the generic is fine.
    The same logic would apply also to Z/nZ, small or large and possible Native.GF(p)

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