search

sisl / GridInterpolations.jl

Multidimensional grid interpolation in arbitrary dimensions
Other
52 stars 12 forks source link

switched to reinterpreting a matrix for performance #21

Closed zsunberg closed 6 years ago

zsunberg commented 6 years ago

@Shushman and @mykelk ,

Since I'll be using this in a few weeks, and I don't want vertices to return a matrix, I decided I'd fix it now so that no-one starts developing with matrix vertices. This also adds a parameter to the grid types encoding the dimensionality of the grid and fixes #15 .

coveralls commented 6 years ago

Coverage Status

Coverage increased (+0.04%) to 97.095% when pulling 0a0dfb5a4f17b5ae90d6274b7d35f26d81b2c3d2 on static2 into 0e6c588a517a201f7f48a481adcf4d3d210d3d99 on master.

zsunberg commented 6 years ago

sorry - the title of this PR is misleading - I forgot to edit it. This is not for performance, but for usability. vertices is better represented as a list of vectors rather than a matrix.

We could argue about whether the constructors should be inner or outer constructors. Shouldn't be too hard to change later though and I need to focus on other stuff.

zsunberg commented 6 years ago

@Shushman can you take a quick look at this? After you do that, I'll merge it (I would "request a code review" from you, but I don't think I can do that until you respond to the request to collaborate).

Shushman commented 6 years ago

Looks good to me (other than potentially addressing Mykel's comments on StaticArrays version?) The change to vertices barely adds any time at all, while achieving the better semantic interpretability.