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

fix: example requires numpy to be imported #1622

Merged
merged 2 commits into from
Jan 7, 2021
Merged

fix: example requires numpy to be imported #1622

merged 2 commits into from
Jan 7, 2021

Conversation

FionnD
Copy link
Contributor

@FionnD FionnD commented Jan 7, 2021

A user copying and pasting the existing sample will hit the following error.

NameError: name 'numpy' is not defined

This is avoidable by including import numpy in the example.

A user copying and pasting the existing sample will hit the following error. 
```
NameError: name 'numpy' is not defined
```

This is avoidable by including import numpy in the example.
@FionnD FionnD requested a review from hanxiao January 7, 2021 11:10
@FionnD FionnD requested a review from a team as a code owner January 7, 2021 11:10
@FionnD FionnD requested a review from CatStark January 7, 2021 11:10
@github-actions
Copy link

github-actions bot commented Jan 7, 2021

This PR closes: #1

JoanFM
JoanFM previously requested changes Jan 7, 2021
README.md Outdated Show resolved Hide resolved
@github-actions
Copy link

github-actions bot commented Jan 7, 2021

Latency summary

Current PR yields:

  • 😶 index QPS at 2120, delta to last 3 avg.: +0%
  • 😶 query QPS at 39, delta to last 3 avg.: +0%

Breakdown

Version Index QPS Query QPS
current 2120 39
0.9.2 2157 39
0.9.1 2172 39
0.8.22 2061 37

Backed by latency-tracking. Further commits will update this comment.

Copy link
Member

@JoanFM JoanFM left a comment

Choose a reason for hiding this comment

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

@alexcg1 do we want to add this or we assume it can be incomplete?

Copy link
Member

@CatStark CatStark left a comment

Choose a reason for hiding this comment

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

Looks good

@JoanFM JoanFM dismissed their stale review January 7, 2021 11:34

Change done

@codecov
Copy link

codecov bot commented Jan 7, 2021

Codecov Report

Merging #1622 (b4953c6) into master (0685b1d) will increase coverage by 0.72%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1622      +/-   ##
==========================================
+ Coverage   84.14%   84.86%   +0.72%     
==========================================
  Files         127      127              
  Lines        6642     6642              
==========================================
+ Hits         5589     5637      +48     
+ Misses       1053     1005      -48     
Impacted Files Coverage Δ
jina/logging/sse.py 92.18% <0.00%> (-3.13%) ⬇️
jina/flow/base.py 86.47% <0.00%> (-0.52%) ⬇️
jina/docker/hubapi.py 86.16% <0.00%> (+7.54%) ⬆️
jina/docker/hubio.py 73.10% <0.00%> (+9.77%) ⬆️

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 0685b1d...975fe62. Read the comment docs.

Copy link
Member

@hanxiao hanxiao left a comment

Choose a reason for hiding this comment

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

it was intentionally removed for saving one-line, example code snippets are for showing capability in a quick & lean way. It is optimized for size, not for reproducibility. For reproducibility, I will use repl.it for all example snippet.

@hanxiao
Copy link
Member

hanxiao commented Jan 7, 2021

will merge for now as it is just one line code change. For other example snippet, please see my comment above

@hanxiao hanxiao merged commit 1485258 into master Jan 7, 2021
@hanxiao hanxiao deleted the FionnD-patch-1 branch January 7, 2021 13:48
@alexcg1
Copy link
Member

alexcg1 commented Jan 7, 2021

Very much agreed with @hanxiao . README should be for showing /how/ code works in general, not showing /exact/ code to pull it off

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants