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 individual scores per attributes of an observation #1

Merged
merged 2 commits into from
Aug 23, 2020

Conversation

trechberger
Copy link
Contributor

Added individual scores per attributes of an observation and adapted example accordingly. README should be adapted as well.

@coveralls
Copy link

coveralls commented Jul 21, 2020

Pull Request Test Coverage Report for Build 35

  • 9 of 14 (64.29%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-2.2%) to 92.424%

Changes Missing Coverage Covered Lines Changed/Added Lines %
coupled_biased_random_walks/detect.py 9 14 64.29%
Totals Coverage Status
Change from base Build 33: -2.2%
Covered Lines: 183
Relevant Lines: 198

💛 - Coveralls

@dkaslovsky
Copy link
Owner

Hi @favouritechord - thanks for the PR! I'll give this a closer look as soon as I can, but it might take a few days before I can commit a bit of time. Thanks for understanding and for contributing.

Copy link
Owner

@dkaslovsky dkaslovsky left a comment

Choose a reason for hiding this comment

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

Hi @favouritechord, thank you again for the contribution! I believe this is a nice addition and that users will appreciate the new functionality.

Here are a few overall comments for your review:

  • One thing I really like about your changes is that we can simplify the calculation of scores using the _value_scores function you've added. What would you think of leveraging your implementation to change the _score function to simply be: return sum(itervalues(self._value_scores(observation)))?
    This way, we only calculate the individual score components once using your implementation. If this is something you'd like to include, please feel free to make the change in your branch and push it up. No problem if you'd rather leave it out though, as I can make the change in a subsequent push.

  • I believe this PR can be ready to be merged after the addition of some unit tests for the CBRW.value_scores() function. These tests can closely follow the existing tests for CBRW.score(). In particular, adding tests that adapt test_score_before_fit, test_score, test_score_unknown_features_default, and test_score_unknown_features_ignore for value_scores() would leave the changes in this PR well tested and ready to merge. I'd love to see these tests added to your PR, but please let me know if this is something you'd rather not implement as I'd be happy to do so.

Let me know what you think of the various comments -- I'm happy to discuss. Thanks again for your time to make this contribution!

example.py Outdated Show resolved Hide resolved
example.py Outdated Show resolved Hide resolved
example.py Outdated Show resolved Hide resolved
example.py Outdated Show resolved Hide resolved
coupled_biased_random_walks/detect.py Outdated Show resolved Hide resolved
coupled_biased_random_walks/detect.py Outdated Show resolved Hide resolved
coupled_biased_random_walks/detect.py Show resolved Hide resolved
@trechberger
Copy link
Contributor Author

Hi @dkaslovsky, thank you for the review and your suggestions. I adapted the code to include all suggestions and your idea to use the sum of the _value_scores for the _score function. Nice!

I did not yet add unit tests, it'd be great if you could add those.

Thanks and thank you again for the repo! Great work.

@dkaslovsky dkaslovsky merged commit 44b354f into dkaslovsky:master Aug 23, 2020
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.

None yet

3 participants