Skip to content
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

More comprehensive CI process #187

Merged
merged 31 commits into from
Mar 6, 2024
Merged

Conversation

adamsardar
Copy link
Contributor

@adamsardar adamsardar commented Feb 27, 2024

  • πŸ‘· CI pipeline now tests against more versions of Python: 3.8 through 3.12
  • πŸ› sparse argument for sklearn.OneHotEncoder is deprecated so we now use sparse_output. Closes [Bug]: OneHotEncodingTransformer no long compatible with sklearnΒ #186 and [Bug]: Tests fail in Py > 3.8Β #184
    • πŸ’š Fix CI build for Python 3.9+ (or rather, for sklearn 1.4+, which is the most recent wheel available for py39+)
    • ⬆️ This has forced an increase in minimum sklearn version to 1.2.0.
  • ✨ CI now runs on all branches, not just main. Developers can receive feedback on their contributions before PR stage.
  • ⚑ Using uv to install dependencies. The 'Install Dependencies' stage has gone from 20 seconds to 4 seconds.
  • βž– Drops black and bandit in favour of a pure ruff setup.
  • ⬆️ Updated ruff to more up-to-date version, unlocking the formatter options to replace black.
  • 🚨 ruff formatter has changed a handful of files alongside ruff autofixes.
  • I realised too late that some of this PR duplicates some efforts in Feature/update pre commitΒ #169 . I managed to get E721 under control using lint.preview = true (see docs), but just added E501 to ignore, so that's still there for Feature/update pre commitΒ #169 to tackle.
  • πŸ“Œ Explicitly setting OS to test on to ubuntu-22.04.

Closes #179.

βž• md and emoji deps for smart pytest results
…arn 1.2 (with no backwards compatibility from 1.4). See #184
…ut for the way that we have tests setup. Plus it adds TWO dependencies.
Copy link
Contributor

@davidhopkinson26 davidhopkinson26 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving as happy with changes. Would still appreciate an answer to my above question re removal of int in some type hints as I'm not clear on why this has been done but can't see a problem with it.

@@ -549,7 +549,7 @@ def __init__(
weights_column: str | None = None,
prior: int = 0,
level: str | list | None = None,
unseen_level_handling: str | int | float | None = None,
unseen_level_handling: str | float | None = None,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

N - is this just because int is redundant or is this a fix for something?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's redundant.

From PEP 484:

Rather than requiring that users write import numbers and then use numbers.Float etc., this PEP proposes a straightforward shortcut that is almost as effective: when an argument is annotated as having type float, an argument of type int is acceptable; similar, for an argument annotated as having type complex, arguments of type float or int are acceptable.

Good chat in this thread.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whilst this PR has been approved - I'll just wait a day or so to check that something unexpected has happened here. We aren't in a rush.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a double check of the code path and am confident that we'll be ok. I'll merge this in.

.github/workflows/python-package.yml Show resolved Hide resolved
.ruff.toml Show resolved Hide resolved
@adamsardar adamsardar merged commit 1ef0ea9 into main Mar 6, 2024
12 checks passed
@davidhopkinson26 davidhopkinson26 deleted the 179-ci-overhaul-modern-tests branch March 21, 2024 09:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: OneHotEncodingTransformer no long compatible with sklearn More comprehensive CI process
2 participants