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

Inefficient conversion from OSCAR to Singular #3828

Open
fingolfin opened this issue Jun 5, 2024 · 5 comments
Open

Inefficient conversion from OSCAR to Singular #3828

fingolfin opened this issue Jun 5, 2024 · 5 comments

Comments

@fingolfin
Copy link
Member

It seems there is quite some inefficient conversion code from OSCAR to Singular (perhaps also the other way around) in e.g. src/Rings/mpoly.jl. One example:

function (SF::Singular.N_AlgExtField)(a::fqPolyRepFieldElem)
  F = parent(a)
  SFa = gen(SF)
  res = SF(coeff(a, 0))
  for i in 1:degree(F)-1
    res += SF(coeff(a, i))*SFa^i # <- very inefficient
  end
  return res
end

At the very least don't compute SFa^i in each iteration, reuse the previous one. But really, we should use the coefficient vector and directly construct the output from that.

Multiple other functions in that file have the exact same problem.

I assume the algebraic geometry / commutative algebra team will want to resolve this.

@ederc
Copy link
Member

ederc commented Jun 5, 2024

Dumb question: Shouldn't these conversion functions not be part of Singular.jl?

@lgoettgens lgoettgens changed the title Inefficient conversion from OSCAR to Singula Inefficient conversion from OSCAR to Singular Jun 5, 2024
@fingolfin
Copy link
Member Author

@ederc agreed

@fingolfin
Copy link
Member Author

Searching for ^i in this file reveals more instances.

@fingolfin
Copy link
Member Author

We also just noticed that there may be a correctness issue here if the field on the OSCAR and Singular sides use different defining polynomials?

It also doesn't even check if the size of the fields coincides

@thofma
Copy link
Collaborator

thofma commented Jun 12, 2024

Should be addressed together with #976

@fingolfin fingolfin removed the triage label Jun 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants