-
Notifications
You must be signed in to change notification settings - Fork 42
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 streaming functionality to Synergistic Forest #537
Conversation
Co-Authored-By: KevinWang905 <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remember to remove the extra print
statements~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
black
formatting is not complete.
Codecov Report
@@ Coverage Diff @@
## staging #537 +/- ##
============================================
- Coverage 92.36% 78.95% -13.41%
============================================
Files 7 7
Lines 419 499 +80
============================================
+ Hits 387 394 +7
- Misses 32 105 +73
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can add unit tests to fix the codecov
issue. Also, replicate the change in 9aaa570 to solve the website rendering.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can remove the network.py
changes if you don't plan to include the complete network update_task
in this PR. I would recommend another PR for that later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nhahn7 you can include the XOR experiments in this PR. Or you can open another one for those.
@PSSF23 Sounds good, I’ll work on adding unit tests and remove the |
Tutorial format is needed. You can focus on unit tests first. |
Optimized sphinx version for website rendering and removed network.py changes
@PSSF23 It looks like its failing due to not recognizing the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nhahn7 That would be too much of a dependency. Instead, we can state that the feature is experimental & modify the CircleCI checks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If classes
is not currently defined as a variable, I would prefer to have the same name for all classes
and input_classes
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update your CircleCI cache from v2
to v3
to resolve the error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As currently there's no way to pass the partial_fit
test, just comment out the test for now.
The notebooks and functions should be in experiments
. And you did not add the notebook to the rst
list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. We are not merging it to staging as the dependency on SDTF would decrease the code coverage. Hope that this will be resolved in the future.
Reference issue
#34 #501
Type of change
Feature Request
What does this implement/fix?
Add streaming functionality for forests with
update_task
function.Additional information
For streaming Synergistic Forests, using
update_task
requires partial fitting from scikit-learn-stream forkNotebook demonstrating SynF vs DF (streaming and batch)
Extended from Add Streaming Function #526 Added streaming capability for ODIN #528