search

Nemocas / Nemo.jl

Julia bindings for various mathematical libraries (including flint2)
http://nemocas.github.io/Nemo.jl/
Other
176 stars 57 forks source link

Ambiguity in QQBarField conversion #1722

Closed lgoettgens closed 2 months ago

lgoettgens commented 2 months ago

Observed in https://github.com/Nemocas/AbstractAlgebra.jl/actions/runs/8740738993/job/23985171446?pr=1677#step:7:934

MethodError: (::QQBarField)(::Polymake.LibOscarNumber.OscarNumberAllocated) is ambiguous.

  Candidates:
    (a::QQBarField)(b)
      @ Nemo ~/work/AbstractAlgebra.jl/AbstractAlgebra.jl/oscar-dev/Nemo/src/calcium/qqbar.jl:1523
    (F::Field)(x::Polymake.LibOscarNumber.OscarNumber)
      @ Oscar ~/work/AbstractAlgebra.jl/AbstractAlgebra.jl/oscar-dev/Oscar/src/PolyhedralGeometry/helpers.jl:217

  Possible fix, define
    (::QQBarField)(::Polymake.LibOscarNumber.OscarNumber)

This error will occur in every OscarCI run until fixed.

I think this was introduced in https://github.com/Nemocas/Nemo.jl/pull/1702, but only exercised once https://github.com/oscar-system/Oscar.jl/pull/3512 was merged.

@fingolfin @thofma should we fix this here or in Oscar? In Nemo, this would be a revert of some parts of #1702. In Oscar, we could add a disambiguation function like proposed in the error message. Which of the two do you prefer?

thofma commented 2 months ago

For these conversions, the parent object should have the say, how things are converted. This is makes it much easier to reason about and avoids these kind of ambiguities.I would argue that https://github.com/oscar-system/Oscar.jl/blob/4d5956d43e5054bf4f2df29be6ac61040c2fdb65/src/PolyhedralGeometry/helpers.jl#L216-L217 should not exist. We do the same for Singular rings and it is a constant struggle and broken by design, see https://github.com/oscar-system/Oscar.jl/issues/976.

lgoettgens commented 2 months ago

Alright, but this seems more like a long-term solution. How do we fix it right now?

thofma commented 2 months ago

Which version of Oscar is effected? Only master?

lgoettgens commented 2 months ago

I think this was introduced in https://github.com/Nemocas/Nemo.jl/pull/1702 (Nemo master, not released yet), but only exercised once https://github.com/oscar-system/Oscar.jl/pull/3512 was merged (into Oscar master, not backported to 1.0).

thofma commented 2 months ago

Then I think we should try to fix it in Oscar first. I will give it a try.

lgoettgens commented 2 months ago

I reverted #1702 in https://github.com/Nemocas/Nemo.jl/pull/1723 to keep CI green for now. We can re-land it in the form of #1725 once this issue here is resolved.

lgoettgens commented 2 months ago

Then I think we should try to fix it in Oscar first. I will give it a try.

We could of course include the change in the breaking Nemo release via https://github.com/Nemocas/Nemo.jl/pull/1725, but then you would need to get a fix until this lands in Oscar in the next few weeks (or we can then put a trivial disambiguiting method for this particular case there)

fingolfin commented 2 months ago

If we are doing the breaking release now anyway, why not include it now? The workaround in Oscar is indeed trivial (a disambiguation method).

But in the end I don't have strong feelings. Getting out the Nemo release with the printing changes is more of a priority

lgoettgens commented 2 months ago

Maybe @thofma can comment?

thofma commented 2 months ago

Sorry, I did not have time to try to give it a try in Oscar. I agree that getting the Nemo release out is the more urgent task.

lgoettgens commented 2 months ago

Then I would suggest that we add this to the breaking release, and add a trivial disambiguate method to Oscar and you can try to fix it there sometime in the future.

fingolfin commented 2 months ago

Since we shipped the generic method in a Nemo release now, and plan to solve it in Oscar, is there any reason to keep this issue open?