search

JuliaAI / MLJScientificTypes.jl

Implementation of the MLJ scientific type convention
MIT License
17 stars 6 forks source link

Add convention for PersistenceDiagram #48

Closed mtsch closed 4 years ago

mtsch commented 4 years ago

This PR adds a convention for scitype(::PersistenceDiagram, ::MLJ) = PersistenceDiagram.

I also added entries into the docs. Let me know if I missed anything.

cc: @ablaom

codecov-io commented 4 years ago

Codecov Report

Merging #48 into dev will decrease coverage by 2.69%. The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev      #48      +/-   ##
==========================================
- Coverage   99.11%   96.41%   -2.70%     
==========================================
  Files           7        7              
  Lines         225      223       -2     
==========================================
- Hits          223      215       -8     
- Misses          2        8       +6
Impacted Files Coverage Δ
src/MLJScientificTypes.jl 100.00% <ø> (ø)
src/convention/scitype.jl 79.31% <50.00%> (-16.99%) :arrow_down:
src/autotype.jl 98.00% <0.00%> (-2.00%) :arrow_down:
src/convention/coerce.jl 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update fd9ff43...0b1eec3. Read the comment docs.

ablaom commented 4 years ago

And can you switch the PR target from master to dev

https://github.com/alan-turing-institute/MLJ.jl/blob/master/CONTRIBUTING.md .

mtsch commented 4 years ago

Just a quick question. If I understand this correctly, Scitype needs to be overloaded for the case when you have an Array of non-concrete types. PersistenceDiagram has no type parameters. Is overloading Scitype still necessary in this case?

ablaom commented 4 years ago

Yes, it is still necessary.

Otherwise scitype(v) will compute the union of all the scitypes of the elements of v, whenever eltype(v) <: PersistentDiagram. This is a long story.

mtsch commented 4 years ago

OK, thanks for the clarification. I added the definition.