Skip to content

Remove unnecessary creations of lists at all#2261

Merged
menshikh-iv merged 2 commits into
piskvorky:developfrom
horpto:extra-copy-list
Dec 14, 2018
Merged

Remove unnecessary creations of lists at all#2261
menshikh-iv merged 2 commits into
piskvorky:developfrom
horpto:extra-copy-list

Conversation

@horpto
Copy link
Copy Markdown
Contributor

@horpto horpto commented Nov 8, 2018

No description provided.

Copy link
Copy Markdown
Owner

@piskvorky piskvorky left a comment

Choose a reason for hiding this comment

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

Beautiful! This set of changes fits very well with Gensim mission and philosophy, thank you.

I remember some constructs have a problem with empty generator, but accept empty list. This is a corner case we have to be the most careful, with these changes. I hope the tests cover it well.

Comment thread gensim/models/ldamodel.py Outdated
logger.info("using symmetric %s at %s", name, 1.0 / self.num_topics)
init_prior = np.asarray([1.0 / self.num_topics for i in xrange(prior_shape)], dtype=self.dtype)
init_prior = np.fromiter((1.0 / self.num_topics for i in xrange(prior_shape)),
dtype=self.dtype, count=prior_shape)
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Hanging indent please, here and elsewhere.

We avoid vertical indent because it leads to inconsistent visual indenting (like here), especially when the lines are edited in the future.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

terro.append(l)
else:
others.append(l)
self.assertTrue(all(['terrorism' not in l for l in others]))
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Why this change?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I thought that this line does not check anything worthwhile. By code it is clear that this check is always true. It's not critical for me, so I return this line back.

@menshikh-iv
Copy link
Copy Markdown
Contributor

ping @horpto, please fix @piskvorky comments and I'll merge current PR (don't forget to merge fresh develop first)

@horpto horpto changed the title remove unnecessary creations of lists at all gensim [DNM] remove unnecessary creations of lists at all gensim Dec 13, 2018
- hanged indent
- return assertTrue line
- fix bug in _get_average_score in python2
- a bit refactor bm25
@horpto horpto changed the title [DNM] remove unnecessary creations of lists at all gensim remove unnecessary creations of lists at all gensim Dec 14, 2018
@menshikh-iv menshikh-iv changed the title remove unnecessary creations of lists at all gensim Remove unnecessary creations of lists at all Dec 14, 2018
@menshikh-iv menshikh-iv merged commit c3d2299 into piskvorky:develop Dec 14, 2018
Comment thread gensim/models/ldamodel.py
logger.info("using symmetric %s at %s", name, 1.0 / self.num_topics)
init_prior = np.asarray([1.0 / self.num_topics for i in xrange(prior_shape)], dtype=self.dtype)
init_prior = np.fromiter((1.0 / self.num_topics for i in xrange(prior_shape)),
dtype=self.dtype, count=prior_shape)
Copy link
Copy Markdown
Owner

@piskvorky piskvorky Dec 14, 2018

Choose a reason for hiding this comment

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

This is still vertical indent.

In gensim, we use hanging indent always, consistently: https://www.python.org/dev/peps/pep-0008/#indentation

@horpto horpto deleted the extra-copy-list branch January 19, 2019 12:07
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