search

JuliaAI / ScientificTypes.jl

An API for dispatching on the "scientific" type of data instead of the machine type
MIT License
96 stars 8 forks source link

`ScientificTypes.scitype` and `ScientificTypesBase.scitype` are not the same function. #181

Closed mtsch closed 2 years ago

mtsch commented 2 years ago

I'm maintaining a package that depends on ScientificTypes and overloads scitype. When I tried bumping the ScientificTypes compat to v3.0.0, the tests failed. It seems like in the new version, ScientificTypes.scitype and ScientificTypesBase.scitype are different functions and overloading the one from this package does not produce the desired effect:

julia> ScientificTypes.scitype
scitype (generic function with 1 method)

julia> ScientificTypesBase.scitype
scitype (generic function with 16 methods)

Is this intended behaviour? I have found a workaround by overloading ScientificTypes.ScientificTypesBase.scitype.

ablaom commented 2 years ago

cc @OkonSamuel

OkonSamuel commented 2 years ago

I'm maintaining a package that depends on ScientificTypes and overloads scitype. When I tried bumping the ScientificTypes compat to v3.0.0, the tests failed. It seems like in the new version, ScientificTypes.scitype and ScientificTypesBase.scitype are different functions and overloading the one from this package does not produce the desired effect:

julia> ScientificTypes.scitype
scitype (generic function with 1 method)

julia> ScientificTypesBase.scitype
scitype (generic function with 16 methods)

Is this intended behaviour? I have found a workaround by overloading ScientificTypes.ScientificTypesBase.scitype.

@mtch Yes, this is intended with the new breaking release, the scitype methods from ScientificTypesBase and ScientificTypes are different. Yeah, overloading ScientificTypes.ScientificTypesBase.scitype would work, but in order to avoid type piracy you are only allowed to over ScientificTypes.ScientificTypesBase.scitype function with double arguments. Below is an example

ScientificTypes.ScientificTypesBase.scitype(MyObject::MyObjectType, ::MyConvention) = ...

I am happy to help out if you don't mind showing the code your working with.

mtsch commented 2 years ago

@OkonSamuel thanks for the clarification! The code in question is here: mtsch/PersistenceDiagrams.j/blob/master/src/scitypes.jl. Let me know if this is ok to do. The PersistenceDiagram is a subtype of AbstractArray, but I don't want MLJ to treat it as such in the default convention.

OkonSamuel commented 2 years ago

@OkonSamuel thanks for the clarification! The code in question is here: mtsch/PersistenceDiagrams.j/blob/master/src/scitypes.jl. Let me know if this is ok to do. The PersistenceDiagram is a subtype of AbstractArray, but I don't want MLJ to treat it as such in the default convention. Yeah, this is okay. Drop the kwargs as that has been removed in 3.0. So you would have


import ScientificTypes: DefaultConvention
const SB = ScientificTypes.ScientificTypesBase

SB.scitype(::PersistenceDiagram, ::DefaultConvention) = PersistenceDiagram

mtsch commented 2 years ago

Sweet, thank you!