similar_type implementation #118

c42f · 2016-05-31T05:00:52Z

As discussed in #116, I've had a crack at replacing similar with similar_type. I think this is more systematic than what we've got now, but it still may not quite be right. I'm open to other ways to define the default implementation of similar_type, but it's clear to me that any default implementation has problems for one case or other. Again, see #116 for some other options.

I also had a go at updating the readme for this, and also to document the types which are defined and address the uncertainty around what Point actually is meant to represent.

It's now no longer necessary to have the versions taking a Type{T} parameter, since the unary version of map now correctly inferrs the output FSA type. (And the comment here was wrong.)

* Rename similar to similar_type since it produces slightly different output. * Implement default similar_type which returns the type unchanged if it exactly matches, and falls back to Vec/Mat otherwise * Implement similar_type for Vec,Point,Mat and tests as necessary.

codecov-io · 2016-05-31T05:05:11Z

Current coverage is 91.33%

Merging #118 into master will increase coverage by 1.37%

@@             master       #118   diff @@
==========================================
  Files            11         11          
  Lines           607        611     +4   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits            546        558    +12   
+ Misses           61         53     -8   
  Partials          0          0

Powered by Codecov. Last updated by b68bc74...9763f27

coveralls · 2016-05-31T05:06:32Z

Coverage increased (+1.4%) to 91.354% when pulling 7b9f80a on c42f:similar-type into b68bc74 on SimonDanisch:master.

SimonDanisch · 2016-05-31T09:13:51Z

Wow, coverage increase and docs! :) Thanks a lot!

c42f · 2016-05-31T10:45:25Z

src/FixedSizeArrays.jl

-similar{FSA <: Mat}(::Type{FSA}, SZ::NTuple{2,Int}) = similar(FSA, eltype(FSA), SZ)
-similar{N,M,S, T}(::Type{Mat{N,M,S}}, ::Type{T}) = Mat{N,M,T}
+similar_type{FSA<:Mat,T}(::Type{FSA}, ::Type{T}, sz::NTuple{2, Int}) = Mat{sz[1], sz[2], T}
+similar_type{N,M,S, T}(::Type{Mat{N,M,S}}, ::Type{T}) = Mat{N,M,T}


Hmm, looks like I missed marking these as @pure, and it's not technically necessary to have the second version here (though it's type stable even in 0.4 in contrast to the first version).

coveralls · 2016-05-31T12:22:26Z

Coverage increased (+1.4%) to 91.326% when pulling 17a7b05 on c42f:similar-type into b68bc74 on SimonDanisch:master.

c42f · 2016-05-31T12:35:19Z

No probs, I also just did a little extra cleanup and removed a couple of extraneous definitions in the service of minimalism.

* Document FSA abstract types which are actually in use (no docs for Mutable FSAs yet, since I'm not sure anybody is using them) * Document concrete FSA types provided by the package * Document the requirement to overload `similar_type` in certian cases.

* Avoid some duplication in similar_type definitions * Move `@pure` somewhere more sensible * Remove similar_type convenience function for simplicity

coveralls · 2016-06-01T01:10:03Z

Coverage increased (+1.4%) to 91.326% when pulling 39a9595 on c42f:similar-type into b68bc74 on SimonDanisch:master.

c42f · 2016-06-01T02:59:21Z

Ah, hang on! @andyferris has just enlightened me to the fact that supertype propagates TypeVar as we actually need to make the default similar_type more intelligent. I'll try another iteration.

coveralls · 2016-06-01T07:12:01Z

Coverage increased (+1.5%) to 91.456% when pulling a2a762d on c42f:similar-type into b68bc74 on SimonDanisch:master.

c42f · 2016-06-01T07:13:08Z

Wow, that rabbit hole turned into at least a mid sized dungeon. similar_type should now work without users having to overload it in nearly all circumstances (thanks for the suggestions @andyferris!)

@andyferris @SimonDanisch - have another look if you have time. Yup, similar_type now looks ugly and a bit fragile, but I think it's about as convenient for users as I can make it.

BTW @SimonDanisch how do you want to do the "looks good to me, merge it" thing for this repo? In other repos people variously use LGTM (looks good to me), +1, etc. (Heh, I recently even saw 🚢 🇮🇹 (:ship: :it:))

Make similar_type() work by default in nearly all circumstances, by introspecting the type tree using fsa_abstract(), supertype() and a bunch of other ugly stuff involving `TypeVar`s. Ugh... just ugh, but I hope it'll make it easier for users. * Remove the specialized similar_type() implementations for concrete types as they're no longer needed * Fix up README to reflect the new implementation * Add some extra tests

coveralls · 2016-06-01T07:25:09Z

Coverage increased (+2.0%) to 91.93% when pulling 34bc334 on c42f:similar-type into b68bc74 on SimonDanisch:master.

SimonDanisch · 2016-06-01T09:16:23Z

The complexity of similar_type makes me wonder, if we shouldn't just offer a macro for creating subtypes of FixedSizeArrays.
This would also make it less breaking, if we introduce changes to the type itself.
But it seems to work like this for now, so from me: 🚢 🇮🇹 ;)
I think we can warm up that discussion, when we actually inherit from AbstractArray ;)

c42f · 2016-06-01T10:27:56Z

Thanks for having a look, yeah the complexity is nasty. The more code I wrote there the less it seemed like a good idea! But on the other hand, people are much less likely to have to define similar_type with the current implementation so if it changes later the disruption will be a lot less.

mschauer · 2016-06-01T13:15:24Z

It's like the "expression problem" which was solved for numbers with multiple dispatch and promote_rules comes back with a vengance for fixed size arrays.

andyferris · 2016-06-01T13:33:20Z

@mschauer Indeed. Note that we can have the same issue for how other types of AbstractArrays interact with each other but that Array is so complete and what to do with SparseArray, etc, is pretty obvious that it hasn't become an issue. However, this package is practically begging users to define their own array types (emphasis on plural) that some logic to how they interact needs to be implemented by the user.

What's in there in this PR seems to be the "closest" generic guess you might come up with, but that's my opinion.

🚢 🇮🇹 👍

c42f and others added 3 commits May 29, 2016 07:59

Remove obsolete versions of map()

2d3d351

It's now no longer necessary to have the versions taking a Type{T} parameter, since the unary version of map now correctly inferrs the output FSA type. (And the comment here was wrong.)

Some tests for FSA not parameterized by size and eltype

0d977cb

c42f reviewed May 31, 2016
View reviewed changes

c42f force-pushed the similar-type branch from 17a7b05 to 39a9595 Compare June 1, 2016 00:52

Chris Foster and others added 2 commits June 1, 2016 10:54

Cleanup similar_type a little

39a9595

* Avoid some duplication in similar_type definitions * Move `@pure` somewhere more sensible * Remove similar_type convenience function for simplicity

c42f force-pushed the similar-type branch from a2a762d to 34bc334 Compare June 1, 2016 07:18

c42f merged commit 34bc334 into SimonDanisch:master Jun 1, 2016

c42f deleted the similar-type branch June 1, 2016 13:57

This was referenced Jun 1, 2016

Allowable FSA subtype parameters and similar() #116

Closed

Promote problems #104

Closed

lstagner mentioned this pull request Aug 29, 2016

similar and traits JuliaLang/julia#18161

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

similar_type implementation #118

similar_type implementation #118

c42f commented May 31, 2016

codecov-io commented May 31, 2016 •

edited

Loading

coveralls commented May 31, 2016 •

edited

Loading

SimonDanisch commented May 31, 2016

c42f May 31, 2016

coveralls commented May 31, 2016 •

edited

Loading

c42f commented May 31, 2016

coveralls commented Jun 1, 2016 •

edited

Loading

c42f commented Jun 1, 2016

coveralls commented Jun 1, 2016 •

edited

Loading

c42f commented Jun 1, 2016

coveralls commented Jun 1, 2016 •

edited

Loading

SimonDanisch commented Jun 1, 2016

c42f commented Jun 1, 2016

mschauer commented Jun 1, 2016

andyferris commented Jun 1, 2016 •

edited

Loading

similar_type implementation #118

similar_type implementation #118

Conversation

c42f commented May 31, 2016

codecov-io commented May 31, 2016 • edited Loading

Current coverage is 91.33%

coveralls commented May 31, 2016 • edited Loading

SimonDanisch commented May 31, 2016

c42f May 31, 2016

Choose a reason for hiding this comment

coveralls commented May 31, 2016 • edited Loading

c42f commented May 31, 2016

coveralls commented Jun 1, 2016 • edited Loading

c42f commented Jun 1, 2016

coveralls commented Jun 1, 2016 • edited Loading

c42f commented Jun 1, 2016

coveralls commented Jun 1, 2016 • edited Loading

SimonDanisch commented Jun 1, 2016

c42f commented Jun 1, 2016

mschauer commented Jun 1, 2016

andyferris commented Jun 1, 2016 • edited Loading

codecov-io commented May 31, 2016 •

edited

Loading

coveralls commented May 31, 2016 •

edited

Loading

coveralls commented May 31, 2016 •

edited

Loading

coveralls commented Jun 1, 2016 •

edited

Loading

coveralls commented Jun 1, 2016 •

edited

Loading

coveralls commented Jun 1, 2016 •

edited

Loading

andyferris commented Jun 1, 2016 •

edited

Loading