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

FIX remove extra conditional statement in decision tree #59

Closed
wants to merge 1 commit into from

Conversation

PSSF23
Copy link
Member

@PSSF23 PSSF23 commented Nov 2, 2023

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Any other comments?

@PSSF23 PSSF23 requested a review from adam2392 November 2, 2023 14:24
Copy link

github-actions bot commented Nov 2, 2023

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

Generated for commit: 47f0320. Link to the linter CI: here

Copy link
Collaborator

@adam2392 adam2392 left a comment

Choose a reason for hiding this comment

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

@PSSF23
Copy link
Member Author

PSSF23 commented Nov 2, 2023

Oh I see. This is used in UnsupervisedTree https://docs.neurodata.io/scikit-tree/dev/generated/sktree.tree.UnsupervisedDecisionTree.html

OK, the documentation says differently, so I didn't see it.

@adam2392
Copy link
Collaborator

adam2392 commented Nov 2, 2023

Oh I see. This is used in UnsupervisedTree https://docs.neurodata.io/scikit-tree/dev/generated/sktree.tree.UnsupervisedDecisionTree.html

OK, the documentation says differently, so I didn't see it.

Ah I see. Feel free to update the PR to fix the documentation then.

@PSSF23
Copy link
Member Author

PSSF23 commented Nov 2, 2023

@adam2392 why is min_samples_split calculation repeated in super()._fit and _build_tree? Shouldn't the local class handle it sufficiently?

@adam2392
Copy link
Collaborator

adam2392 commented Nov 2, 2023

@adam2392 why is min_samples_split calculation repeated in super()._fit and _build_tree? Shouldn't the local class handle it sufficiently?

Oh yeah I think in unsupervised tree, that can be removed then since we rely on the fork's check.

I think that was there because I tried adding it into the downstream unsup class, but it felt hacky to me.

@PSSF23
Copy link
Member Author

PSSF23 commented Nov 2, 2023

I'll open another PR if other problems are found.

@PSSF23 PSSF23 closed this Nov 2, 2023
@PSSF23 PSSF23 deleted the fix_tree branch November 6, 2023 17:49
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