-
Notifications
You must be signed in to change notification settings - Fork 15
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
π:bug::snake: Fix bug for 0 passing predictions #341
Conversation
πβ¬οΈ Pin scikit-learn to >=1.3 :snake::art: Replace conda commands with mamba in Makefile :fire::arrow_down: Remove hdbscan (now available in scikit-learn >= 1.3)
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## dev #341 +/- ##
==========================================
- Coverage 27.59% 27.31% -0.29%
==========================================
Files 45 50 +5
Lines 5349 5730 +381
==========================================
+ Hits 1476 1565 +89
- Misses 3873 4165 +292
Flags with carried forward coverage won't be shown. Click here to find out more.
β View full report in Codecov by Sentry. |
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.
It seems like the test coverage for the code is good so assuming the tests are good then the changes are fine and seem to fix the builds. Should get another pair of eyes to look but seems good
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.
Code is fine. Going to need to search and replace mamba/conda in the Docs though
May need to look at hdbscan in tests Autometa/tests/environment.yml Line 14 in 8ebd7a7
|
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.
Looks good, minor comments:
- What happens if the user has
conda
but notmamba
? I created an environment without mamba and the installation was using(PYTHON_INTERPRETER) -m pip install -q virtualenv virtualenvwrapper
. So maybe specify in the docs or somewhere to installmamba
if the user needs the advantageconda
offers. - On the environment with mamba the installation worked well ππΌ
- As @chasemc mentioned, update the docs telling user to use
mamba
and changescikit-learn
intests/environment.yml
- I didn't try running clustering using
hdbscan
but I assume it should work as the tests are passing.
π₯ Remove virtualenv commands from Makefile resolves #331
πβ¬οΈ Pin
scikit-learn
to>=1.3
ππ¨ Replace
conda
commands withmamba
inMakefile
π₯β¬οΈ Remove
hdbscan
(now available in scikit-learn >= 1.3)HDBSCAN has been failing in pytests and this seems to be an artifact of hdbscan being integrated into scikit-learn.
Some of these issues have cropped up in the past few weeks (scikit-learn-contrib/hdbscan#600).
scikit-learn is now pinned to >=1.3 as this is the earliest release of HDBSCAN being available via
sklearn.cluster.HDBSCAN
The
run_hdscan
function has now been updated according to the API that was adopted in thesklearn
library.PR checklist