-
Notifications
You must be signed in to change notification settings - Fork 5
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
Random forest #67
Random forest #67
Conversation
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.
Nice PR @KorenMary! I added a few comments here and there to small changes but it's looking good!
I encountered an error when running by clicking "Generate Labels": sklearn.exceptions.NotFittedError: This RandomForestClassifier instance is not fitted yet. Call 'fit' with appropriate arguments before using this estimator.
Please make sure this is addressed!
src/server/test/test_integration.py
Outdated
if "classifier" in attrs: | ||
assert(model.classifier.loss<0.4) | ||
attrs_classifier = model.classifier.__dict__.keys() | ||
if "loss" in attrs_classifier: | ||
assert(model.classifier.loss<0.4) | ||
if "metric" in attrs_classifier: | ||
assert(model.classifier.metric>0.4) | ||
if "metric" in attrs: | ||
assert(model.metric>0.1) | ||
if "loss" in attrs: |
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.
This confuses me and isn't very clean. Currently we only have loss and metric for CustomCellposeModel and CellClassifierFCNN. Suggestion to have cleaner approach:
- Always only check model.loss and model.metric (i.e. remove if "classifier" in attrs check).
- In models.py add loss and metric for all classes. For those who have a segmentor and a classifier part: model.loss=(classifier.loss + segmentor.loss)/2 and same for metric. For CustomCellposeModel add self.loss_fn to the init of to the class and uncomment line where we compute it.
src/server/dcp_server/models.py
Outdated
masks_classes, | ||
masks_instances, | ||
noise_intensity = self.train_config["classifier"]["train_data"]["noise_intensity"], | ||
max_patch_size = self.train_config["classifier"]["train_data"]["patch_size"]) | ||
|
||
if self.classifier_class == "RandomForest": | ||
patches = create_dataset_for_rf(patches, patch_masks) |
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.
can we rename patches to something more explicit? here this is now features not patches so it is confusing.
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #67 +/- ##
==========================================
+ Coverage 85.61% 86.37% +0.75%
==========================================
Files 20 21 +1
Lines 1314 1431 +117
==========================================
+ Hits 1125 1236 +111
- Misses 189 195 +6 ☔ View full report in Codecov by Sentry. |
… integration test.
No description provided.