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

apply index_type on value #204

Merged
merged 1 commit into from
Sep 9, 2021
Merged

apply index_type on value #204

merged 1 commit into from
Sep 9, 2021

Conversation

piever
Copy link
Collaborator

@piever piever commented Sep 4, 2021

Fix #203 by running the index_type computation by recursing tuples rather than tuple types.

@oschulz I think the basic mistake was to compute index_type on types rather than values. Does this PR fix your type instability problem?

@ranocha this PR should not regress the performance improvements you introduced in #194, but I guess it'd be good to double check it in practice if you have a simple way to run the performance benchmarks on Trixi based on this PR

@ranocha
Copy link
Contributor

ranocha commented Sep 5, 2021

This PR doesn't seem to impact the performance of the benchmark setups I looked at in #194.

@oschulz
Copy link
Contributor

oschulz commented Sep 6, 2021

@piever thanks, I'll try the PR!

I'm currently using this workaround in BAT.jl:

_structvector_indexstyle(contents::Tuple) = _structvector_indexstyle_impl(map(IndexStyle, map(typeof, contents)))
_structvector_indexstyle_impl(::NTuple{N,IndexLinear}) where N = Int
_structvector_indexstyle_impl(::NTuple{N,IndexStyle}) where N = CartesianIndex{1}

It also uses the values - it's a bit too simple (limited to vectors), but maybe the basic map-IndexStyle-over-contents approach could get rid of some of the recursion in the updated StructArrays code in this PR?

@oschulz
Copy link
Contributor

oschulz commented Sep 8, 2021

@piever sorry for the delay - did a few additional tests - looks like this PR fixed the problem completely. Thanks!

@piever
Copy link
Collaborator Author

piever commented Sep 9, 2021

Great, thanks for double checking!

@piever piever merged commit f3bef32 into master Sep 9, 2021
@piever piever deleted the pv/index_type branch September 9, 2021 08:05
@JoaoAparicio
Copy link

Now index_type some times takes values:

index_type(components::NamedTuple) = index_type(values(components))

and some times takes types:
index_type(::Type{LazyRows{T, N, C, I}}) where {T, N, C, I} = I

Seems strange.

@piever
Copy link
Collaborator Author

piever commented Dec 6, 2023

It's true that it is a bit confusing. It turns out that we can just use Base.IndexStyle directly and it seems to infer fine, I've opened #288 to address that.

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.

StructArray ctor type instability
4 participants