gensim icon indicating copy to clipboard operation
gensim copied to clipboard

Handle optional parameters without implicit bool cast

Open nk-fouque opened this issue 1 year ago • 2 comments

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.

nk-fouque avatar Oct 31 '23 15:10 nk-fouque

@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 avatar Apr 08 '24 03:04 mpenkov

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

piskvorky avatar Apr 08 '24 09:04 piskvorky

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

piskvorky avatar Jun 11 '24 12:06 piskvorky

LGTM. Thank you @nk-fouque !

mpenkov avatar Jun 11 '24 14:06 mpenkov