Closed jw3126 closed 1 year ago
Ok from my point of view, this PR is ready!
Thank you so much for all the work! I'v been meaning to look into exactly these things myself, but had not found the time. The changes all make sense, but I'm slightly surprised that moving an inner constructor to the outside has any kind of performance impact. Maybe I've missed the point, as I've only briefly skimmed over the changes.
I'll merge the PR when I find the time to do a bit of a more in-depth review. I have also planned to make a new release soon with all the accumulated improvements.
The changes all make sense, but I'm slightly surprised that moving an inner constructor to the outside has any kind of performance impact.
Ok I did not explain this very well. First of all, simply moving a constructor out does not have any performance impact. However moving the inner constructor out gives us the option to not use it. Instead of
MultiVector(CA, BI, coeffs)
we can directly call
MultiVector{CA, T, BI, K}(coeffs)
The latter is trivial simple to infer, while the former has a subtle issue.
The issue is, that the former constructor is not type stable. BI = (1,2)
vs BI = (2,3)
will give different return types, for the same input type.
Generally speaking, julia does an awesome job at inferring type stable code. Code that requires constant propagation to figure out the type does not infer as well. Although const prop gets better with every release. In most places the old inner constructor worked for me on julia 1.9, while it failed on julia 1.6. Generally, my strategy is to keep things simple stupid and avoid reliance on fancy compiler heuristics, like const prop.
I'll merge the PR when I find the time to do a bit of a more in-depth review. I have also planned to make a new release soon with all the accumulated improvements.
Awesome I wanted to ask you about a release after this is merged anyway.
Fixes various inference and performance issues. The most extreme one:
In an actual use case that involves 3d PGA this gives me a nice speedup: