JuliaAI / MLJModelInterface.jl

Lightweight package to interface with MLJ
MIT License
37 stars 8 forks source link

Export `scitype` #130

Closed ablaom closed 2 years ago

ablaom commented 2 years ago

Addresses #129

ablaom commented 2 years ago

@OkonSamuel If you are happy with this please merge, make a PR onto master and tag a new release directly.

codecov-commenter commented 2 years ago

Codecov Report

Merging #130 (8f3e040) into dev (66c24fa) will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##              dev     #130   +/-   ##
=======================================
  Coverage   87.83%   87.83%           
=======================================
  Files           8        8           
  Lines         296      296           
=======================================
  Hits          260      260           
  Misses         36       36           
Impacted Files Coverage Δ
src/MLJModelInterface.jl 100.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 66c24fa...8f3e040. Read the comment docs.

OkonSamuel commented 2 years ago

@OkonSamuel If you are happy with this please merge, make a PR onto master and tag a new release directly.

@ablaom I guess the reason for not exporting the scitype function from ScientificTypesBase is because it's the two argument definition i.e scitype(X, Convention) whereas the single argument definition is now defined in ScientificTypes.jl and is distinct from the one in ScientifictypesBase. So I guess this is breaking. But I think I have a work around that can still allow us export this function but avoid being a breaking change.

scitype(X) = scitype(get_interface_mode(), X)
scitype(::LightInterface, X) = throw(ArgumentError("FullInterface is needed..."))

Then at MLJBase we define something like.

scitype(::FullInterface, X ) = ScientificTypes.scitype(X)
ablaom commented 2 years ago

@OkonSamuel Thanks for that. Thanks for the clarification. I see this is trickier than I realised.

You are saying that:

  1. This PR, as is, will break MLJBase, yes?
  2. If we add to this PR the code suggested above, then this PR will still break MLJBase, but the fix is not too bad, namely the second bit of code you suggest above, yes?
  3. If both these changes are made, we will have successfully patched the inadvertent breaking behaviour introduced by MLJModelInterface 1.3.4 (failing to export a previously exported method) yes?

If all yes, this sounds like a good plan, so long as:

A. We have backwards compatibility: All MLJBase 0.18.* are compatible with the new MLJModelInteface 1.3.5. B. We yank MLJBase 0.19 and 0.19.1 (already released) from General, after the new patch MLJBase 0.19.2

OkonSamuel commented 2 years ago
  • This PR, as is, will break MLJBase, yes?
  • If we add to this PR the code suggested above, then this PR will still break MLJBase, but the fix is not too bad, namely the second bit of code you suggest above, yes?
  • If both these changes are made, we will have successfully patched the inadvertent breaking behaviour introduced by MLJModelInterface 1.3.4 (failing to export a previously exported method) yes?

@ablaom all yes.

ablaom commented 2 years ago

Closed in favour of #133