JuliaAI / ScientificTypesBase.jl

Base interface for dispatching on the "scientific" type of data instead of the machine type
MIT License
9 stars 4 forks source link

Proposal for different handling of scitype conventions #17

Open ablaom opened 3 years ago

ablaom commented 3 years ago

Currently the possibility of allowing for different scitype conventions is handled by dispatch on a Convention object C, returned by convention() whose value can be changed by the user:

scitype(X;    kw...) = scitype(X, convention();     kw...)

@OkonSamuel, who has been profiling key ScientificTypes performance, has pointed out to me that this is overkill: It allows for the possibility of changing one's convention at run time (for which it is very hard to imaging a use case) but at the cost of some type-instability related loss in performance. It is therefore suggested that a convention should just means a set of overloadings of scitype(X; kw...) (instead of scitype(X, MyConvention(); kw...)) and choosing a convention means choosing the package which does the overloading (which commits the user to the convention once the package is imported).

Apart from performance benefits, this would also simplify code.

Although this is a breaking change, as far as I know this would currently effect only ScientificTypes - the only convention-providing package I know of.

Still, I would like to flag this in case there are objections to moving forward or other suggestions.

cc @OkonSamuel @juliohm @tlienart

ablaom commented 3 years ago

Okay, I did not properly understand and communicate some details here, as @OkonSamuel has pointed out to me. We still keep the idea of a convention object but:

(i) Eliminate the convention() function with mutable return value (ii) Make ScientificTypesBase.jl surrender ownership of the single argument scitype(X) function to convention-providing packages, such as ScientificTypes.jl ,and articulate the basic scitype rules (eg, scitype of a tuple is Tuple of the scitypes) in terms of the two argument function scitype(X, C::Convention) (iii) Require the convention-providing package to:

@OkonSamuel Is that the gist of it?

OkonSamuel commented 3 years ago

@OkonSamuel Is that the gist of it?

Yes.