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

Ambiguity in QQBarField conversion #1722

Closed
lgoettgens opened this issue Apr 18, 2024 · 12 comments · Fixed by oscar-system/Oscar.jl#3667
Closed

Ambiguity in QQBarField conversion #1722

lgoettgens opened this issue Apr 18, 2024 · 12 comments · Fixed by oscar-system/Oscar.jl#3667

Comments

@lgoettgens
Copy link
Collaborator

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 #1702, but only exercised once oscar-system/Oscar.jl#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
Copy link
Member

thofma commented Apr 18, 2024

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 oscar-system/Oscar.jl#976.

@lgoettgens
Copy link
Collaborator Author

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

@thofma
Copy link
Member

thofma commented Apr 19, 2024

Which version of Oscar is effected? Only master?

@lgoettgens
Copy link
Collaborator Author

lgoettgens commented Apr 19, 2024

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

@thofma
Copy link
Member

thofma commented Apr 19, 2024

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

@lgoettgens
Copy link
Collaborator Author

I reverted #1702 in #1723 to keep CI green for now. We can re-land it in the form of #1725 once this issue here is resolved.

@lgoettgens
Copy link
Collaborator Author

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 #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
Copy link
Member

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
Copy link
Collaborator Author

Maybe @thofma can comment?

@thofma
Copy link
Member

thofma commented Apr 30, 2024

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
Copy link
Collaborator Author

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
Copy link
Member

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?

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 a pull request may close this issue.

3 participants