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

Added new ValueError in place of assertion error for no model data provided in lsi model #3271

Merged
merged 12 commits into from
Mar 22, 2022

Conversation

mark-todd
Copy link
Contributor

@mark-todd mark-todd commented Nov 26, 2021

Added warning to lsi model for initialising a model with no data

Fixes #3270.

…ovided in lsi model

Added warning to lsi model for initialising a model with no data
gensim/models/lsimodel.py Outdated Show resolved Hide resolved
gensim/models/lsimodel.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Nov 26, 2021

Codecov Report

Merging #3271 (9741a81) into develop (ac3bbcd) will decrease coverage by 0.05%.
The diff coverage is 55.55%.

@@             Coverage Diff             @@
##           develop    #3271      +/-   ##
===========================================
- Coverage    81.43%   81.38%   -0.06%     
===========================================
  Files          122      122              
  Lines        21052    21107      +55     
===========================================
+ Hits         17144    17177      +33     
- Misses        3908     3930      +22     
Impacted Files Coverage Δ
gensim/utils.py 71.29% <53.84%> (-0.69%) ⬇️
gensim/models/lsimodel.py 63.88% <60.00%> (-0.16%) ⬇️
gensim/downloader.py 79.67% <0.00%> (-1.65%) ⬇️
gensim/matutils.py 77.07% <0.00%> (-1.07%) ⬇️
gensim/models/fasttext.py 89.73% <0.00%> (-0.23%) ⬇️
gensim/test/test_keyedvectors.py 99.29% <0.00%> (+0.02%) ⬆️
gensim/models/keyedvectors.py 82.43% <0.00%> (+0.19%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ac3bbcd...9741a81. Read the comment docs.

@piskvorky
Copy link
Owner

@mpenkov could you please check why one of the builds is failing? Thank you!

@piskvorky piskvorky added this to the Next release milestone Feb 19, 2022
@mark-todd
Copy link
Contributor Author

I've now corrected the file according to your suggested modifications - hope this is all ok!

@mark-todd
Copy link
Contributor Author

Not sure why it's still saying one change requested - the import line missing issue should now be resolved

gensim/models/lsimodel.py Outdated Show resolved Hide resolved
@piskvorky piskvorky self-assigned this Feb 25, 2022
Copy link
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.

We follow PEP8 & PEP257 in code style: no blank line after docstring; blank line after import group; more than one space before comment etc.

Plus types doesn't seem to be defined in utils.py. Does this code really work, did you try it?

gensim/utils.py Show resolved Hide resolved
gensim/utils.py Outdated Show resolved Hide resolved
gensim/models/lsimodel.py Outdated Show resolved Hide resolved
gensim/models/lsimodel.py Show resolved Hide resolved
mark-todd and others added 4 commits February 25, 2022 19:20
Added space

Co-authored-by: Radim Řehůřek <me@radimrehurek.com>
Added import space

Co-authored-by: Radim Řehůřek <me@radimrehurek.com>
Added space after False

Co-authored-by: Radim Řehůřek <me@radimrehurek.com>
@piskvorky
Copy link
Owner

piskvorky commented Mar 18, 2022

Looks good, thanks!

@mpenkov what's the best way to shut up our failing flake8 CI test about this?

Screen Shot 2022-03-18 at 13 03 04

I'd prefer to have the variable there and named – makes the code much easier to read and understand. I don't really care that the variable is unused, so what.

Other than that LGTM.

@piskvorky piskvorky assigned mpenkov and unassigned piskvorky Mar 18, 2022
@mpenkov
Copy link
Collaborator

mpenkov commented Mar 22, 2022

Merging. Thank you for your contribution and your patience @mark-todd !

@mpenkov mpenkov merged commit a4808c1 into piskvorky:develop Mar 22, 2022
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.

Update: assert self.projection.u is not None, "decomposition not initialized yet"
3 participants