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

[ENH] Add basic MORF implementation and design #21

Merged
merged 18 commits into from
Feb 8, 2023
Merged

[ENH] Add basic MORF implementation and design #21

merged 18 commits into from
Feb 8, 2023

Conversation

adam2392
Copy link
Collaborator

@adam2392 adam2392 commented Feb 3, 2023

Towards #20

Need to also think about the design of the Cython API to allow future possibilities. Also porting over the oblique tree and related examples.

Before submitting

  • I've read and followed all steps in the Making a pull request
    section of the CONTRIBUTING docs.
  • I've updated or added any relevant docstrings following the syntax described in the
    Writing docstrings section of the CONTRIBUTING docs.
  • If this PR fixes a bug, I've added a test that will fail without my fix.
  • If this PR adds a new feature, I've added tests that sufficiently cover my new functionality.

After submitting

  • All GitHub Actions jobs for my pull request have passed.

Signed-off-by: Adam Li <adam2392@gmail.com>
Signed-off-by: Adam Li <adam2392@gmail.com>
Signed-off-by: Adam Li <adam2392@gmail.com>
Signed-off-by: Adam Li <adam2392@gmail.com>
Signed-off-by: Adam Li <adam2392@gmail.com>
Signed-off-by: Adam Li <adam2392@gmail.com>
Signed-off-by: Adam Li <adam2392@gmail.com>
Signed-off-by: Adam Li <adam2392@gmail.com>
Signed-off-by: Adam Li <adam2392@gmail.com>
@adam2392
Copy link
Collaborator Author

adam2392 commented Feb 8, 2023

@jshinm I will probably merge this in once I get the CIs working, but you may be interested in reviewing just the files in sktree/tree/tests/test_tree.py and docs/api.rst to i) get an idea of the breadth of unit-tests we want to see and ii) start brain-storming documentation.

The rest of the files are irrelevant unless you are interested in trying to improve the MORF trees.

Signed-off-by: Adam Li <adam2392@gmail.com>
Signed-off-by: Adam Li <adam2392@gmail.com>
Signed-off-by: Adam Li <adam2392@gmail.com>
Signed-off-by: Adam Li <adam2392@gmail.com>
Signed-off-by: Adam Li <adam2392@gmail.com>
Signed-off-by: Adam Li <adam2392@gmail.com>
@codecov
Copy link

codecov bot commented Feb 8, 2023

Codecov Report

Base: 0.00% // Head: 83.66% // Increases project coverage by +83.66% 🎉

Coverage data is based on head (ebebdf4) compared to base (961b284).
Patch coverage: 83.80% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##            main      #21       +/-   ##
==========================================
+ Coverage   0.00%   83.66%   +83.66%     
==========================================
  Files          2        8        +6     
  Lines        101      557      +456     
==========================================
+ Hits           0      466      +466     
+ Misses       101       91       -10     
Impacted Files Coverage Δ
sktree/ensemble/_supervised_forest.py 30.55% <30.55%> (ø)
sktree/ensemble/_unsupervised_forest.py 71.85% <71.85%> (ø)
sktree/__init__.py 77.27% <75.00%> (ø)
sktree/tree/_classes.py 88.78% <89.00%> (+88.78%) ⬆️
sktree/ensemble/__init__.py 100.00% <100.00%> (ø)
sktree/tests/test_forest.py 100.00% <100.00%> (ø)
sktree/tree/__init__.py 100.00% <100.00%> (ø)
sktree/tree/tests/test_tree.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@adam2392
Copy link
Collaborator Author

adam2392 commented Feb 8, 2023

An issue seems that there is something up w/ Patch Oblique tree that fails on occasion:

=========================== short test summary info ============================
[406](https://github.com/neurodata/scikit-tree/actions/runs/4128531247/jobs/7133086102#step:14:407)
FAILED sktree/tree/tests/test_tree.py::test_sklearn_compatible_estimator[PatchObliqueDecisionTreeClassifier(random_state=12)-check_sample_weights_invariance(kind=ones)] - AssertionError: 
[407](https://github.com/neurodata/scikit-tree/actions/runs/4128531247/jobs/7133086102#step:14:408)
Not equal to tolerance rtol=1e-07, atol=1e-09
[408](https://github.com/neurodata/scikit-tree/actions/runs/4128531247/jobs/7133086102#step:14:409)
For PatchObliqueDecisionTreeClassifier sample_weight=None is not equivalent to sample_weight=ones
[409](https://github.com/neurodata/scikit-tree/actions/runs/4128531247/jobs/7133086102#step:14:410)
Mismatched elements: 1 / 16 (6.25%)
[410](https://github.com/neurodata/scikit-tree/actions/runs/4128531247/jobs/7133086102#step:14:411)
Max absolute difference: 1
[411](https://github.com/neurodata/scikit-tree/actions/runs/4128531247/jobs/7133086102#step:14:412)
Max relative difference: 1.
[412](https://github.com/neurodata/scikit-tree/actions/runs/4128531247/jobs/7133086102#step:14:413)
 x: array([1, 1, 1, 1, 2, 2, 2, 2, 1, 1, 1, 1, 2, 2, 2, 2])
[413](https://github.com/neurodata/scikit-tree/actions/runs/4128531247/jobs/7133086102#step:14:414)
 y: array([1, 1, 1, 1, 2, 2, 2, 2, 1, 1, 1, 1, 2, 2, 2, 1])
[414](https://github.com/neurodata/scikit-tree/actions/runs/4128531247/jobs/7133086102#step:14:415)
============= 1 failed, 262 passed, 34 skipped in 69.56s (0:01:09) =============

This could be due to resolving ties, or something, but it seems that it occurs only on 1 of the samples. Moreover, taking a look at https://github.com/scikit-learn/scikit-learn/blob/7970d78cfc1f8e705a8f417f29bb47b08e1bf0aa/sklearn/utils/estimator_checks.py#L1040, it seems that the last samples are essentially the same as the rest, so this could be a numerical issue from breaking ties or something.

Might be a deeper Cython issue where we need to figure out where some ties might occur.

Signed-off-by: Adam Li <adam2392@gmail.com>
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.

None yet

1 participant