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

Evaluation tool #66

Closed
wants to merge 15 commits into from
Closed

Conversation

KirillTim
Copy link
Contributor

While working on #39 I faced many issues and decided to start big refactoring.
Add cache(100MB in ram for all archived signatures) to speedup signature access operations.
Add downloads cache to reduce network operations time.
Introduce abstract Algorithm class and start working on slow reference implementation of WMDistanceModel, which will be base line for all future models
Comments are welcome :)

@KirillTim
Copy link
Contributor Author

@ibrahimsharaf I will try not to change anything related to you work in existing code to simplify future merge. But you can have a look at my classes hierarchy to make you algorithm compatible with mine

@codecov-io
Copy link

codecov-io commented Apr 2, 2017

Codecov Report

Merging #66 into master will increase coverage by 1.74%.
The diff coverage is 74.54%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #66      +/-   ##
==========================================
+ Coverage   39.72%   41.47%   +1.74%     
==========================================
  Files          10       14       +4     
  Lines        1772     4239    +2467     
==========================================
+ Hits          704     1758    +1054     
- Misses       1068     2481    +1413
Impacted Files Coverage Δ
utils.py 34.22% <100%> (-0.15%) ⬇️
refactored/cache.py 55.1% <55.1%> (ø)
refactored/models/base.py 33.48% <88.09%> (ø)
tests/test_cache.py 90.9% <90.9%> (ø)
tests/__init__.py 45.72% <0%> (ø)

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 bfe9de3...7b0daf2. Read the comment docs.

def build(stream):
"""build from downloaded archives"""

# TODO: is it possible to have different `signature`s for one `proto_signature` ?
Copy link
Owner

Choose a reason for hiding this comment

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

No, there should be only one signature for each proto_signature (that is basically a stack trace).

@marco-c
Copy link
Owner

marco-c commented Apr 9, 2017

Overall, it looks good so far.

Is the cache actually needed? How slow is it to rebuild it? If it isn't slow, we could remove the cache altogether (it's better to avoid premature optimizations).

@KirillTim
Copy link
Contributor Author

KirillTim commented Apr 15, 2017

@marco-c Yes, the cache is actually needed. It dramatically speedup experiments evaluating and network operations. I can profile it and publish speedup results for my desktop machine, if you wish.

How slow is it to rebuild it?

It cost no time to rebuilt the cache, while it is built during reading. The only problem is disk space. Model+corpus take ~600mb of disk space

@KirillTim KirillTim changed the title Evaluation tool [WIP] Evaluation tool Apr 15, 2017
@KirillTim
Copy link
Contributor Author

KirillTim commented Apr 15, 2017

Evaluation tool seems to be done.
We also have baseline solution based on cosine distance now. IMO, #29 can be closed now.
#11 can be closed as well.
In future we will need to extract evaluation into function to simplify progress measuring.

We have fully working prototype which produce compact human readable(and understandable) results! 😎

UPD: Oh, there is still some work to be done.
It is not reasonable to write unit tests for testing model accuracy, but I can write some parts of evaluation tool logic as unit tests. Should I add it here or on the new PR, after this will be merged(I prefer the second option)?

@marco-c
Copy link
Owner

marco-c commented Apr 19, 2017

I really like the changes, but could you split them in separate PRs?

One PR to refactor the code with the classes; one PR to refactor the downloader; one PR to introduce the cache; one PR to add the basic evaluation tool.

Also, put everything in the top source directory and not in a refactored subdirectory, and remove the old code.

It is not reasonable to write unit tests for testing model accuracy, but I can write some parts of evaluation tool logic as unit tests. Should I add it here or on the new PR, after this will be merged(I prefer the second option)?

Yes, this can be an additional and separate PR.

@marco-c
Copy link
Owner

marco-c commented Apr 19, 2017

@marco-c Yes, the cache is actually needed. It dramatically speedup experiments evaluating and network operations.

The only problem is that for the evaluation we will use periods of time far from each other, so the cache might not help much in those cases.

@KirillTim
Copy link
Contributor Author

KirillTim commented Apr 20, 2017

The only problem is that for the evaluation we will use periods of time far from each other, so the cache might not help much in those cases.

Yes, for sure. Cache won't help much in production, and will be turned off in the future. But while we are in stage of active development it will help us :)

This was referenced Apr 20, 2017
This was referenced Jun 20, 2017
@KirillTim
Copy link
Contributor Author

Last part of this PR is implemented in #88

@KirillTim KirillTim closed this Jun 24, 2017
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