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

Remove ad hoc arithmetic functions #365

Merged
merged 1 commit into from Feb 12, 2021

Conversation

fingolfin
Copy link
Member

The promotion system takes care of these

... unless these were added for other reasons that I am not aware of, such as performance improvements? @wbhart might know?

The promotion system takes care of these
@wbhart
Copy link
Contributor

wbhart commented Jan 22, 2021

Ad hoc arithmetic requires separate implementations for Julia types because of limitations of the Julia type inference. It can cause hangs, crashes, stack overflows and ambiguity errors without them. Generally they are needed, but it is often not easy to trigger the problems in test cases and the problems vary from version to version of Julia.

The underlying reason is to do with the way Julia prioritises concrete types over abstract types and unions and how the latter are prioritised over each other (or not, as the case may be). By putting explicit functions in for each concrete type (this only needs to be done for Julia types, e.g. Int, BigInt, BigFloat, etc.), the type inference has fewer problems, ambiguities don't occur and the associated hangs, crashes and stack overflows go away.

I'll take a look and see which ones you want to remove. Probably some of them will fall into this category.

(Also, having the functions there makes it easy for people to improve them in any given instance later if performance is important. I recognise that is a trade-off with code simplicity, but it is only a secondary consideration.)

@wbhart
Copy link
Contributor

wbhart commented Jan 22, 2021

At a minimum, do not remove the ones where one argument is a Julia type. In all cases it would be helpful if you could add a test case checking that the call they were meant to intercept still works, though I should like to point out that this may not be sufficient to detect the problems that can occur, as they usually only happen deep within code where the type inference is already pretty stressed.

@rfourquet
Copy link
Contributor

rfourquet commented Jan 22, 2021

I can't help much here, I experimented also needing to define similar methods to not run into ambiguity errors. The AbstractAlgebra docs give a good guideline on which methods should be implemented. But in any case, I don't think inference has anything to do here: the called method is always chosen depending on the exact type of the argument, whether it's statically known (if inference was precise) or determined at runtime (involving dynamic dispatch); in particular, which method is called doesn't depend on the result of inference.

@fingolfin
Copy link
Member Author

I have added tests to PR #364 which should (?) cover all (?) the adhoc rules removed here; let's focus on getting that PR there in, then I can rebase this to see if / how it fails, or not.

@fingolfin fingolfin closed this Jan 22, 2021
@fingolfin fingolfin reopened this Jan 22, 2021
@fingolfin
Copy link
Member Author

Rebased so the newly added tests are in effect

@thofma
Copy link
Collaborator

thofma commented Feb 12, 2021

Lets merge and see how this plays out.

@thofma thofma merged commit c5a48f2 into oscar-system:master Feb 12, 2021
@fingolfin fingolfin deleted the mh/remove-adhoc-arith branch February 12, 2021 10:23
fingolfin added a commit to fingolfin/Singular.jl that referenced this pull request Jun 6, 2023
The promotion system takes care of these
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.

None yet

4 participants