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

Allow features to be AbstractVector (in addition to AbstractMatrix) #150

Open
wants to merge 9 commits into
base: dev
Choose a base branch
from

Conversation

sanderbboisen
Copy link

I was playing around with DecisionTree.jl for a potential project and found that I could not produce a regression tree from a single feature. I have made changes to allow features to be a vector in the build_tree functions.

I have tested the changes using RunTest.jl with Julia 1.6.4 .

@ablaom ablaom changed the base branch from master to dev May 6, 2022 01:05
@ablaom ablaom changed the title Vec or mat support Allow features to be AbstractVector (in addition to AbstractMatrix) May 6, 2022
@ablaom
Copy link
Member

ablaom commented May 6, 2022

I don't see any harm in this enhancement, apart from a mild complication in the codebase.

Anyone else have objections?

@sanderbboisen Would you be willing to add a test of native and ScikitLearn API with a vector X ?

@sanderbboisen
Copy link
Author

I don't see any harm in this enhancement, apart from a mild complication in the codebase.

Anyone else have objections?

@sanderbboisen Would you be willing to add a test of native and ScikitLearn API with a vector X ?

Sure, I will get on it. Incoming update.

@codecov-commenter
Copy link

codecov-commenter commented May 16, 2022

Codecov Report

❗ No coverage uploaded for pull request base (dev@17cb46d). Click here to learn what that means.
The diff coverage is n/a.

@@          Coverage Diff           @@
##             dev     #150   +/-   ##
======================================
  Coverage       ?   88.84%           
======================================
  Files          ?        9           
  Lines          ?      986           
  Branches       ?        0           
======================================
  Hits           ?      876           
  Misses         ?      110           
  Partials       ?        0           

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 17cb46d...070e86e. Read the comment docs.

@sanderbboisen
Copy link
Author

I ran into an issue as several functions like apply_something were overloaded so that any Vector inputs worked differently. These are essentially only called by theapply_something functions that users are supposed to call. I have taken the path of least modification, and named them _apply_something. This way users only have access to the functions they are intended to use, and using Vectors as inputs work.

I have added tests to tests/scikitlearn.jl and everything seems to work.

I am sorry for the confusing commit structure, things got a little messy at some point.

I think there is a general issue with the way that the module is constructed, and I have been refactoring it in a separate branch on my fork. For one, the structure does not allow multiple usage of utils.jl and there is generally some issues with code repetition iirc. I will open another PR on this when I am done refactoring.

@ablaom
Copy link
Member

ablaom commented Jun 1, 2022

I am happy with this now.

I suspect this smaller PR may create a few merge conflicts for the more complicated #166, so let's wait for:

before merging this one.

@sanderbboisen Thanks for your contribution and patience.

@ablaom
Copy link
Member

ablaom commented Jul 1, 2022

@sanderbboisen Would you now be happy to rebase your PR?

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.

4 participants