Skip to content
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

Hard deps and CSV->DelimitedFiles #78

Merged
merged 7 commits into from
Nov 6, 2019
Merged

Hard deps and CSV->DelimitedFiles #78

merged 7 commits into from
Nov 6, 2019

Conversation

tlienart
Copy link
Collaborator

@tlienart tlienart commented Nov 5, 2019

This PR

  • removes optional dependencies
  • uses the stdlib DelimitedFiles to load boston, etc

See also JuliaAI/ScientificTypes.jl#38; this addresses #77 ; ideally merge this in dev second (after merging JuliaAI/ScientificTypes.jl#38)

@tlienart tlienart changed the base branch from master to dev November 5, 2019 17:46
src/datasets_requires.jl Outdated Show resolved Hide resolved
test/runtests.jl Outdated Show resolved Hide resolved
Copy link
Member

@ablaom ablaom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many thanks for all the work. Just need the commented tests returned, thanks

@tlienart
Copy link
Collaborator Author

tlienart commented Nov 5, 2019

ok done, actually I'm glad you spotted it because the code for the datasets wasn't properly included 🙄 should be all good now

@ablaom ablaom mentioned this pull request Nov 5, 2019
3 tasks
@ablaom
Copy link
Member

ablaom commented Nov 5, 2019

It looks like the macro forms of loading data still import CSV:

macro load_boston()
    quote
        import CSV
        y, X = unpack(load_boston(), ==(:MedV), x->x != :Chas)
        (X, y)
    end
end

(As a side note: the raison d'etre for using macros - importing an optional dependency - is gone. Let's keep the macro - which has a different return value from the functions - because otherwise we break MLJ)

@tlienart
Copy link
Collaborator Author

tlienart commented Nov 5, 2019

thanks yes will remove those

load_iris() = load_dataset("iris.csv", COERCE_IRIS)
load_crabs() = load_dataset("crabs.csv", COERCE_CRABS)

load_crabs()

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this fixes the travis fail, but line 166 is presumably not supposed to be here.

Copy link
Member

@ablaom ablaom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed an extraneous line of code. See above

@tlienart tlienart merged commit c55296e into dev Nov 6, 2019
@ablaom ablaom deleted the perf branch April 28, 2020 06:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants