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

Replace the contents of this repo with the contents of MLJScientificTypes, and remove MLJ-specific content #126

Closed juliohm closed 3 years ago

juliohm commented 3 years ago

@ablaom this PR should take care of most issues. There are 2 tests still failing, could you please give a hand?

Also, let me know if you have a better name for the default convention, I used DefaultConvention but we could use something shorter too if desired. Default or Canonical or Community convention could be alternatives.

Before we merge this into the dev branch, which I am assuming is the main branch of development, we need to check that:

[x] the guid of the package is correct
[x] the name of the default convention is ok
[x] all tests are passing on github

juliohm commented 3 years ago

Just to double check, this is what I did in this PR:

] dev ScientificTypes.jl
] dev MLJScientificTypes.jl
copied contents of MLJScientificTypes.jl to ScientificTypes
adjusted the contents to remove MLJ-specific explanations

DilumAluthge commented 3 years ago

I like DefaultConvention, since it helps combat the potential misunderstanding that the convention is MLJ-specific.

DilumAluthge commented 3 years ago

Just to make this PR title easier to understand, I would rename the title of this PR with "replace the contents of this repo with the contents of MLJScientificTypes, and remove MLJ-specific content".

That way, when we are looking at the commit history in the future, it will be easier to quickly figure out what this PR did.

juliohm commented 3 years ago

Thank you all for the prompt updates. I've accepted the commit suggestions by @OkonSamuel and have incorporated the suggestions by @ablaom and @DilumAluthge on the README.md and Project.toml.

ablaom commented 3 years ago

@juliohm , can you please bump the [compat] for StatisticalTraits:

StatisticalTraits = "2"

The earlier patch release was breaking MLJBase and is being yanked. Version 2 is already tagged. https://github.com/alan-turing-institute/StatisticalTraits.jl/issues/12

juliohm commented 3 years ago

I've bumped the version of StatisticalTraits and copied the Acknowledgements section to the bottom of the README as suggested.

Please let me know if something else is missing. Let's see if tests will pass.

ablaom commented 3 years ago

@juliohm Can you please fix the build fail. The problem looks similar to the one diagosed by @OkonSamuel earlier.

juliohm commented 3 years ago

@OkonSamuel could you please give a hand with the last fixes? I am super packed this week working on presentations (including JuliaCon) and can't look into this issue quickly.

codecov-commenter commented 3 years ago

Codecov Report

Merging #126 (f1ce740) into dev (6b05dd9) will increase coverage by 0.92%. The diff coverage is 98.59%.

@@            Coverage Diff             @@
##              dev     #126      +/-   ##
==========================================
+ Coverage   97.67%   98.59%   +0.92%     
==========================================
  Files           2        7       +5     
  Lines          43      285     +242     
==========================================
+ Hits           42      281     +239     
- Misses          1        4       +3

Impacted Files	Coverage Δ
src/convention/scitype.jl	`96.66% <96.66%> (ø)`
src/schema.jl	`97.14% <97.14%> (ø)`
src/coerce.jl	`98.27% <98.27%> (ø)`
src/convention/coerce.jl	`98.96% <98.96%> (ø)`
src/ScientificTypes.jl	`100.00% <100.00%> (+5.26%)`	:arrow_up:
src/autotype.jl	`100.00% <100.00%> (ø)`
src/init.jl	`100.00% <100.00%> (ø)`
... and 3 more

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 6b05dd9...f1ce740. Read the comment docs.

ablaom commented 3 years ago

@juliohm @OkonSamuel Thanks for your help with this PR.