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

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

nk-fouque
Copy link

@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.

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.

None yet

3 participants