Skip to content

Julia v1.6+ overhaul #163

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

Merged
merged 7 commits into from
Dec 3, 2021
Merged

Julia v1.6+ overhaul #163

merged 7 commits into from
Dec 3, 2021

Conversation

dkarrasch
Copy link
Member

@dkarrasch dkarrasch commented Nov 2, 2021

This additional annotation seems necessary on current master. Otherwise it fails on map tuples longer than some number between 10 (which passes) and 13 (which fails), see our most recent commits.

EDIT: This PR started because one test in the block maps test suite started to fail on nightly/v1.8. It turned out that some functions that are called in the process of constructing BlockMaps had type inference difficulties. Apparently, quite some work was done even in the v1.6-v1.8 period regarding type inference in Julia Base, and due to changes pre-v1.6 it seemed impossible to maintain clear code that would equally run in a type-stable fashion on all Julia versions starting from v1.0. Given that v1.6 is gonna be the new LTS once v1.7 is released, I ended up removing all the VERSION branches and switched to by now well-established syntax. Lifting the lower compat bound to v1.6 shouldn't do much harm, as we are leaving a fully operational version behind, just in case some downstream code really requires Julia v1.0.

There is no rush to merge this, and we can leave it open for a while and see if we want to merge other urgent features before we lift the lower compat bound.

@codecov
Copy link

codecov bot commented Nov 2, 2021

Codecov Report

Merging #163 (08cc5f2) into master (e575b5f) will increase coverage by 0.21%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #163      +/-   ##
==========================================
+ Coverage   99.43%   99.65%   +0.21%     
==========================================
  Files          14       14              
  Lines        1061     1157      +96     
==========================================
+ Hits         1055     1153      +98     
+ Misses          6        4       -2     
Impacted Files Coverage Δ
src/LinearMaps.jl 100.00% <100.00%> (+2.27%) ⬆️
src/blockmap.jl 99.04% <100.00%> (-0.04%) ⬇️
src/composition.jl 100.00% <100.00%> (ø)
src/kronecker.jl 98.43% <100.00%> (+0.03%) ⬆️
src/linearcombination.jl 100.00% <100.00%> (ø)
src/wrappedmap.jl 100.00% <100.00%> (ø)
src/show.jl 100.00% <0.00%> (ø)
src/scaledmap.jl 100.00% <0.00%> (ø)
src/transpose.jl 100.00% <0.00%> (ø)
src/conversion.jl 100.00% <0.00%> (ø)
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e575b5f...08cc5f2. Read the comment docs.

@dkarrasch
Copy link
Member Author

I wonder if there's something to be investigated here. It has to do with the length of the map tuple, and it's not quite clear where inference breaks down, or which threshold could be responsible for that.

@dkarrasch dkarrasch requested a review from Jutho November 2, 2021 12:23
@Jutho
Copy link
Collaborator

Jutho commented Nov 2, 2021

I have not been following Julia / master development the last months, so no idea what causes this. Are both tuples no longer inferred? Could it help to add some parametric type declaration to the arguments of the function, to force the compiler to specialise? Maybe there is a heuristic in place by which this function is no longer specialised on the type of its arguments?

@dkarrasch
Copy link
Member Author

I'm investigating now, but that is work in progress. It seems like it's puzzling the compiler to do Base.setindex on a tuple and rebind the variable to the new tuple. I'm trying to rewrite the rowcolranges function using map, ntuple and broadcasting to construct the needed objects directly.

@dkarrasch dkarrasch changed the title add type annotation in rowcolranges Make concatenation more type stable Nov 3, 2021
@dkarrasch
Copy link
Member Author

Hm, my solution requires cumsum on tuples, which was implemented only in Julia v1.5. How do we feel about lifting the Julia lower bound to v1.6, the assumed next LTS version? After all, this PR fixes an inference issue that came up only on v1.8. Alternatively, we could include Compat.jl as a dependency, but that feels strange given that Julia v1.0 has been unmaintained for quite a while. If people agree, I'd use this PR for a little version cleanup.

@dkarrasch dkarrasch changed the title Make concatenation more type stable Julia v1.6+ overhaul Nov 4, 2021
@dkarrasch dkarrasch closed this Dec 3, 2021
@dkarrasch dkarrasch reopened this Dec 3, 2021
@dkarrasch dkarrasch merged commit 7363170 into master Dec 3, 2021
@dkarrasch dkarrasch deleted the dk/inference branch December 3, 2021 16:15
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.

2 participants