Skip to content
This repository was archived by the owner on Feb 28, 2025. It is now read-only.

removed n_clusters from elbow_curve #247

Merged
merged 18 commits into from
Jan 29, 2023

Conversation

WSShawn
Copy link
Contributor

@WSShawn WSShawn commented Jan 26, 2023

Describe your changes

removed n_clusters from elbow_curve()

Issue ticket number and link

Closes #190

Checklist before requesting a review

  • I have performed a self-review of my code
  • I have added thorough tests (when necessary).
  • I have added the right documentation in the docstring and changelog (when needed)

📚 Documentation preview 📚: https://sklearn-evaluation--247.org.readthedocs.build/en/247/

@WSShawn
Copy link
Contributor Author

WSShawn commented Jan 26, 2023

still working on fixing the test_clustering.oy file

@edublancas
Copy link
Contributor

as a general tip: open the PR until it's ready. if you want to open a PR for early feedback or to test the CI, mark is as a draft

@coveralls
Copy link

Pull Request Test Coverage Report for Build 4015018253

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.1%) to 94.297%

Files with Coverage Reduction New Missed Lines %
src/sklearn_evaluation/telemetry.py 2 96.92%
Totals Coverage Status
Change from base Build 4010433489: 0.1%
Covered Lines: 2745
Relevant Lines: 2911

💛 - Coveralls

@coveralls
Copy link

coveralls commented Jan 26, 2023

Pull Request Test Coverage Report for Build 4017940789

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.05%) to 94.223%

Files with Coverage Reduction New Missed Lines %
src/sklearn_evaluation/plot/clustering.py 2 98.36%
Totals Coverage Status
Change from base Build 4010433489: 0.05%
Covered Lines: 2675
Relevant Lines: 2839

💛 - Coveralls

@idomic
Copy link
Contributor

idomic commented Jan 26, 2023

@WSShawn the CI is failing, converting to draft

@idomic idomic marked this pull request as draft January 26, 2023 15:38
@WSShawn
Copy link
Contributor Author

WSShawn commented Jan 26, 2023

Sorry for all the commits, I'll mark it as draft first next time. Also, do we have any experience dealing with the Tcl installation error before? I'm not sure what that means. @edublancas @idomic

@edublancas
Copy link
Contributor

tcl is a conda error, trigger the CI again to fix it

@WSShawn
Copy link
Contributor Author

WSShawn commented Jan 26, 2023

@edublancas need some help with the readthedocs error.

@edublancas
Copy link
Contributor

the error is: _queue.Empty. I've seen this before, it's an intermittent error in the jupyter-client, nothing we can do but to rebuild the docs. i re-triggered the build https://readthedocs.org/projects/sklearn-evaluation/builds/19287048/

@WSShawn
Copy link
Contributor Author

WSShawn commented Jan 26, 2023

Thanks! Let's see how it goes this time.

@WSShawn
Copy link
Contributor Author

WSShawn commented Jan 26, 2023

I think it's the same error again

@edublancas
Copy link
Contributor

ok, looked at the error. I think the real reason is the timeout - there is one cell in a notebook that's taking longer than expected. check out the jupyter-book documentation, I remember seeing how to extend the cell timeout; if that doesn't work, then we'd need to investigate why this is failing. perhaps we can reduce the runtime of that cell to fix it

@WSShawn
Copy link
Contributor Author

WSShawn commented Jan 26, 2023

@edublancas found this about cell execution timeout. Is this what we want? If yes, which file did we use to set up sphinx? Thanks!

@gtduncan
Copy link
Contributor

Having the same issue, seems to be set in docs/conf.py? (looking at execution_timeout = 30)

@WSShawn
Copy link
Contributor Author

WSShawn commented Jan 26, 2023

Thanks! Any specific value did you use to fix this? @gtduncan

@gtduncan
Copy link
Contributor

It seems to fail even when I set it fairly high (120 seconds), maybe it's something wrong with the cell

@WSShawn
Copy link
Contributor Author

WSShawn commented Jan 27, 2023

@edublancas could this cell be trying to extract n_clusters?

Here is a preview of the cell contents:

['# models with their corresponding parameters', 'params = [{', " 'model': 'sklearn.ensemble.RandomForestRegressor',", " 'params': {", " 'n_estimators': 50"]
...
["files = [f'{i}.ipynb' for i in ids]", '', '# execute notebooks using papermill', 'for f, p in zip(files, params):', " pm.execute_notebook('train.ipynb', output_path=f, parameters=p)"]

@idomic
Copy link
Contributor

idomic commented Jan 27, 2023

@WSShawn please rebase on master and let me know if this is ready for review.

@WSShawn WSShawn force-pushed the remove_depraction_warning branch from 8ebac33 to e2ef0ec Compare January 27, 2023 18:54
@edublancas
Copy link
Contributor

remember to also delete the deprecation message in the docstring, the one that begins with .. deprecated:: 0.9

@WSShawn
Copy link
Contributor Author

WSShawn commented Jan 27, 2023

ok, I'll delete it tonight after my class

@WSShawn WSShawn marked this pull request as ready for review January 28, 2023 03:08
@edublancas edublancas merged commit eb8f0fa into ploomber:master Jan 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deprecation: elbow_curve will change its signature in version 0.9
5 participants