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

Handle optional parameters without implicit bool cast #3502

Merged
merged 2 commits into from
Jun 11, 2024

Conversation

nk-fouque
Copy link
Contributor

@nk-fouque nk-fouque commented Oct 31, 2023

This PR updates the code in tfidfmodel.py that used "if [param]" to check if param had been defined. This method used an implicit casting to boolean to treat None as False, however that posed a few issues.

Mainly when using Pandas Series, which have a particular way of handling boolean casting (raises a warning prompting to use a defined method), despite the fact that they are proper Iterable Objects and should be allowed as parameters.

Other parts of the library already use "if [param] is not None", I just aligned the code to those parts.

Proper unit testing has been run to ensure this is not a breaking change.

@mpenkov
Copy link
Collaborator

mpenkov commented Apr 8, 2024

@piskvorky What are your thoughts on this?

Personally, I'm OK with the code base as-is (explicit is better than implicit, etc.), unless there is a demonstrable use case (e.g. a test case) that shows that it's broken.

@mpenkov mpenkov added this to the Next release milestone Apr 8, 2024
@piskvorky
Copy link
Owner

Sure, makes sense to me. I don't know if this breaks anything deeper down – but hopefully our tests would catch that.

@mpenkov mpenkov modified the milestone: Summer 2024 release Jun 11, 2024
@piskvorky
Copy link
Owner

@mpenkov could you run the test suite? And if all passes, let's merge & release as part of 4.3.3.

@mpenkov
Copy link
Collaborator

mpenkov commented Jun 11, 2024

LGTM. Thank you @nk-fouque !

@mpenkov mpenkov merged commit c95bdb4 into piskvorky:develop Jun 11, 2024
32 checks passed
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.

3 participants