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

Fix type instability in unflattening Tuples #67

Merged
merged 6 commits into from
Feb 6, 2024

Conversation

simsurace
Copy link
Member

On master this does not infer:

julia> tpl = (1., ((2., 3.), 4.));
julia> par, unflatten = flatten(tpl);
julia> @code_warntype unflatten(par)

This PR fixes this and (so I believe) improves the type inference with a recursive implementation and a fallback for empty tuples.

src/flatten.jl Outdated Show resolved Hide resolved
src/flatten.jl Outdated Show resolved Hide resolved
test/flatten.jl Outdated Show resolved Hide resolved
@willtebbutt
Copy link
Member

Thanks for opening this. Busy morning -- will try to review later today

Copy link

codecov bot commented Feb 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (567b070) 97.36% compared to head (f5bdf67) 97.40%.
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #67      +/-   ##
==========================================
+ Coverage   97.36%   97.40%   +0.03%     
==========================================
  Files           8        8              
  Lines         228      231       +3     
==========================================
+ Hits          222      225       +3     
  Misses          6        6              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

simsurace and others added 2 commits February 6, 2024 11:56
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Copy link
Member Author

@simsurace simsurace left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is your view on using views (pun intended)?

@willtebbutt
Copy link
Member

I think views definitely make sense here. Very open to that.

While you do that, I'm going to drop 1.5, because we really don't need it.

@simsurace
Copy link
Member Author

Thinking it over a bit, I think going to views should be evaluated a bit more carefully, as it also requires generalizing a lot of the signatures of the unflatten functions. I will keep that for a later PR perhaps.

@simsurace
Copy link
Member Author

I'm gonna suggest approving this as is. I bumped the version. Maybe we can release #68 in the same swoop?

@simsurace simsurace requested a review from willtebbutt February 6, 2024 17:44
@willtebbutt willtebbutt closed this Feb 6, 2024
@willtebbutt willtebbutt reopened this Feb 6, 2024
@willtebbutt
Copy link
Member

Thinking it over a bit, I think going to views should be evaluated a bit more carefully, as it also requires generalizing a lot of the signatures of the unflatten functions. I will keep that for a later PR perhaps.

Good point. Yeah, let's defer that until later. I do think it would be good to look at though, because we're currently doing a lot of copying.

Copy link
Member

@willtebbutt willtebbutt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

I'd really quite like to stick to the standard 1-version-per-PR rule though, so please tag this separately from #68

@simsurace
Copy link
Member Author

No problem!

@simsurace simsurace merged commit ccfdc13 into master Feb 6, 2024
14 of 15 checks passed
@simsurace simsurace deleted the fix-flatten-tuple-type-instability branch February 6, 2024 18:22
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