-
Notifications
You must be signed in to change notification settings - Fork 8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Exclude certain kinds of dictionary tables from being interpreted as tables #189
Conversation
According to local testing, this PR will resolve JuliaAI/MLJText.jl#25 . |
@bkamins Be great if you could have a look at this, thanks. |
Codecov Report
@@ Coverage Diff @@
## dev #189 +/- ##
==========================================
+ Coverage 93.50% 93.54% +0.04%
==========================================
Files 6 6
Lines 431 434 +3
==========================================
+ Hits 403 406 +3
Misses 28 28
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Apart from a comment related to JuliaData/Tables.jl#309 I have a general design question that I think would be worth discussing. The issue is that
This means that is we see The problem is that some MLJ.jl users can use a table for which You know the MLJ.jl ecosystem much better than me 😄, so I do not feel qualified to give informed proposals what to do with this, but clearly the sentence:
is not true currently. It probably should be made more precise (if nothing is changed) to saying that table is only considered something for which |
Given the discussion in JuliaData/Tables.jl#309 and Tables.jl 1.10.0 release the PR should be updated to |
@bkamins Thanks for the review and comments. I have now updated the excluded key type from
I have clarified the ScientificTypes.jl documentation to that effect, and raised an issue to do the same in MLJ docs. As far as allowing tables in MLJ for which |
This PR will:
String
keysString
keysscitype
docstring (scitype
docstring should include link to convention summary and/or a synopsis #185)