Refactor: Remove Cython - Reduce tech debt #346
Conversation
eddiebergman
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #346 +/- ##
==========================================
+ Coverage 73.54% 78.91% +5.37%
==========================================
Files 29 45 +16
Lines 2831 4767 +1936
Branches 629 1005 +376
==========================================
+ Hits 2082 3762 +1680
- Misses 632 806 +174
- Partials 117 199 +82 ☔ View full report in Codecov by Sentry. |
eddiebergman
changed the title
|
FYI: I tested running DeepCave with ConfigSpace 0.8.0 and ran into the following problems when executing :
|
|
@mfeurer, I did a major push on getting the documentation updated to @sarah-segel thanks for the report back, I will look into them with the next big push. Do you have any more information on the first one other than "ran into an error"? Second one is likely a bug on my behalf, will investigate. Last one is good to know about. It was a private method so I considered safe enough to remove. I will add it back in with the deprecation warning. |
|
@sarah-segel should have been fixed now, can you try again? |
| - name: "Install dependancies" | ||
| run: python -m pip install -e ".[dev]" | ||
| - name: "Build Docs" | ||
| run: mkdocs build --clean --strict |
There was a problem hiding this comment.
Hey, is there a reason you removed the pushing of the documentation?
There was a problem hiding this comment.
Yup, we moved away from Sphinx and I didn't know how to automate doc deployments given this setup. I will document how to do it in manual way in CONTRIBUTING.md
.github/workflows/pytest.yml
Outdated
| @@ -49,12 +47,6 @@ jobs: | |||
| matrix: | |||
| python-version: ["3.8", "3.9", "3.10", "3.11", "3.12"] | |||
| os: ["ubuntu-latest", "macos-latest", "windows-latest"] | |||
| exclude: | |||
| ORDERABLE: ClassVar[bool] = True # Let ConfigSpace know there is an order to the values | ||
|
|
||
| alpha: float | ||
| """Some docstring decsription of this attribute.""" |
There was a problem hiding this comment.
Should the docstring be further at the top of this class?
There was a problem hiding this comment.
Nope, it's alright right here. It puts the documentation right with the declaration.
test/test_conditions.py
Outdated
| @@ -49,6 +49,10 @@ | |||
| from ConfigSpace.hyperparameters.hyperparameter import Hyperparameter | |||
|
|
|||
|
|
|||
| def some_new_function(): | |||
There was a problem hiding this comment.
What do we need this one for?
There was a problem hiding this comment.
Good spot, removed
|
|
||
|
|
||
| def center_range( | ||
| center: int, |
There was a problem hiding this comment.
What happens if the difference from the lower to the center is smaller than from the center to higher? Is this an issue?
There was a problem hiding this comment.
Nope, roundrobin() will start just giving those from the above_center only after it can no longer yield equally between below_center and above_center
| """Initialize a hyperparameter. | ||
|
|
||
| Args: | ||
| name: |
There was a problem hiding this comment.
Is it necessary to duplicate this information here?
There was a problem hiding this comment.
Not entirely but I don't have it a different way. This is documenting parameters, while the other ones are documenting attributes. I would say it's necessary as it shows up in editors as help information
| The probability density of the values. Where values are not legal, | ||
| the probability density is zero. | ||
| """ | ||
| # TODO(eddiebergman): Backwards compatible restriction, why this restriction? |
There was a problem hiding this comment.
This would be great to know ;)
There was a problem hiding this comment.
Will leave it in now unless it's a problem at some point
|
Will be making a release once these tests pass :) |
This PR attempts to first convert everything to Python, so a type checker can be run across everything as well as use linters properly. This also includes updating everything to pytest.
Note: This PR is unreviewable. The strategy has been to minimally change tests while refactoring to ensure tested behaviour still passes. Some notable exceptions are where functionality was removed or sampling strategy was changed and the expected values differ. Unfortunatly the formatter has rendered them also unreviewable.
Update: All SMAC unittests passed without code changes so it seems mostly backwards compatible.
Some notable changes:
Category([True, False, "None"])(Now handled in follow up feat: Allow arbitrary values as categoricals/constants/ordinals #376 )TODO:
Configuration(space, vector, validate=False)to deal with Problems integrating with BOHB #75HyperparameterNotFoundErrorprohibits.getfrom functioning as expected #348self.get_orderinget_neighbors#344NoneinCategoricalHyperparameters #166numberargument inget_neighbors#288is_legalcan be made more explicit #237check_defaulttoConstantfor uniformity with other hyperparameters #270valueofUniformFloatHyperparametershould be in [0, 1] #290ufhpofUniformIntegerHyperparameterpublic likeNormalIntegerHyperparameter#291get_neighbourscan generate n samples and then validate them #236get_neighbors#239get_neighboursandto_uniform#200CategoricalHyperparameter#133