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

Update argument order in compose #1521

Closed
wants to merge 3 commits into from

Conversation

lgoettgens
Copy link
Collaborator

This updates all definitions and uses of compose in Nemo according to oscar-system/Oscar.jl#690 to follow compose(f,g) = g(f(x)).

This change is obviously breaking.

@lgoettgens
Copy link
Collaborator Author

lgoettgens commented Aug 10, 2023

Cf. the discussion in Nemocas/AbstractAlgebra.jl#1025

@codecov
Copy link

codecov bot commented Aug 10, 2023

Codecov Report

Merging #1521 (0a29061) into master (38fd074) will increase coverage by 0.65%.
Report is 6 commits behind head on master.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1521      +/-   ##
==========================================
+ Coverage   82.75%   83.40%   +0.65%     
==========================================
  Files          95       95              
  Lines       37186    39032    +1846     
==========================================
+ Hits        30773    32556    +1783     
- Misses       6413     6476      +63     
Files Changed Coverage Δ
src/arb/ComplexPoly.jl 79.53% <ø> (ø)
src/arb/RealPoly.jl 79.35% <ø> (ø)
src/arb/acb_poly.jl 77.23% <ø> (ø)
src/arb/arb_poly.jl 78.38% <ø> (ø)
src/flint/fmpq_poly.jl 94.93% <ø> (+0.31%) ⬆️
src/flint/fmpz_mod_poly.jl 94.86% <ø> (ø)
src/flint/fq_default_poly.jl 93.50% <ø> (ø)
src/flint/fq_nmod_poly.jl 93.02% <ø> (ø)
src/flint/fq_poly.jl 97.04% <ø> (ø)
src/flint/nmod_poly.jl 90.02% <ø> (ø)
... and 2 more

... and 15 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@fingolfin
Copy link
Member

The breakage this introduces is of a worse kind than other such PRs, because instead of causing compile errors, it may result in some code actually continuing to work but return wrong results. I have no brilliant solution for this, though, other than perhaps it would be good to add compose_left and compose_right (but then who can remember which is which); or precompose / postcompose (but again I am not sure if people can remember; plus these are harder to find for tab completion). Hrm.

If we had compose_left / compose_right (or whatever), then code could be converted to use them, making remaining uses of compose stand out more. Then we could rip off the bandaid in two steps: in one we disable compose completely for a release (to catch any remaining code using it), and then in a later release could add it back...

(Personally I think what I could work with best myself is to use * and $\circ$ operators: I have been drilled since high school that $(f\circ g)(x) = f(g(x))$, and later learned that $x^{fg}=x^{f\cdot g} = x^{fg} = (x^f)^g$, i.e., $(fg)(x) = g(f(x))$. However, besides the obvious issue due to use of unicode, I also suspect that many won't be familiar with one or both of these, too.

Hrm. Sorry, no great solution from me here. We should still resolve this eventually, somehow!

@thofma
Copy link
Member

thofma commented Aug 24, 2023

Could we perhaps keep the discussion at oscar-system/Oscar.jl#690?

@lgoettgens
Copy link
Collaborator Author

Making this a draft to prevent accidental merge

@lgoettgens
Copy link
Collaborator Author

too many conflicts. closing

@lgoettgens lgoettgens closed this Apr 3, 2024
@lgoettgens lgoettgens deleted the lg/compose branch April 3, 2024 12:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants