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

Clean up *_mat_entry_set calls #1910

Merged
merged 7 commits into from
Oct 27, 2024
Merged

Conversation

lgoettgens
Copy link
Collaborator

@lgoettgens lgoettgens commented Oct 18, 2024

Use the already installed setindex! methods with @inbounds instead of ccalls everywhere

a, i - 1, j - 1, u, base_ring(a))
@ccall libflint.fq_mat_entry_set(
a::Ref{FqPolyRepMatrix}, (i-1)::Int, (j-1)::Int, u::Ref{FqPolyRepFieldElem}, base_ring(a)::Ref{FqPolyRepField}
)::Nothing
end

@inline function setindex!(a::FqPolyRepMatrix, u::ZZRingElem, i::Int, j::Int)
Copy link
Collaborator Author

@lgoettgens lgoettgens Oct 18, 2024

Choose a reason for hiding this comment

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

for this function, I would like to make use of set! from #1908 (instead of the ccall).
The problem is that the set! implemented there needs to access parent of the first arg, so it does not work with pointers there. What do you think about adding a third argument to set! that defaults to parent(first_arg) (for non-pointers), but in this particular case we can just pass base_ring(a)

Just to point this out, this won't happen in this PR.

Copy link
Member

Choose a reason for hiding this comment

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

yeah, makes sense. Should the parent be a thread argument, though, or maybe a kwarg (that would make it easier to grep, harder to get confused about?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

what do you mean by thread argument?

And the way that I understand how kwargs work in julia, a call always involves a NamedTuple construction and another call (to the kwcall) function. This may have an overhead that we do not want to have in something low-level as setindex!.

Copy link
Member

Choose a reason for hiding this comment

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

I vote for making it a normal positional argument.

Copy link
Member

Choose a reason for hiding this comment

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

sorry, typo/braino, I meant "third argument"

@lgoettgens lgoettgens marked this pull request as ready for review October 18, 2024 16:59
Copy link

codecov bot commented Oct 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.38%. Comparing base (633112d) to head (46babf0).
Report is 11 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1910      +/-   ##
==========================================
+ Coverage   87.24%   87.38%   +0.14%     
==========================================
  Files          97       97              
  Lines       35590    35491      -99     
==========================================
- Hits        31049    31014      -35     
+ Misses       4541     4477      -64     

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

src/flint/FlintTypes.jl Outdated Show resolved Hide resolved
@lgoettgens
Copy link
Collaborator Author

I only added Base.require_one_based_indexing to all places without a _check_dim call. For the majority, Nemocas/AbstractAlgebra.jl#1879 will take care of that.

@fingolfin fingolfin merged commit c8e30fc into Nemocas:master Oct 27, 2024
24 checks passed
@lgoettgens lgoettgens deleted the lg/mat_entry_set branch October 27, 2024 21:26
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