-
Notifications
You must be signed in to change notification settings - Fork 48
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
Structure Refactor and Duplicate Trait Support #75
base: main
Are you sure you want to change the base?
Conversation
hooks: | ||
- id: flake8 | ||
additional_dependencies: [flake8-bugbear, pep8-naming] | ||
# - repo: https://gitlab.com/pycqa/flake8 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you need to fix it in github actions as well , otherwise the build fails.
) -> list[AttributeStatistic]: | ||
return [ | ||
{ | ||
**attr, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's not use ** syntax , i think it's hard to read
from open_rarity.models.collections import AttributeCounted, AttributeStatistic | ||
|
||
|
||
def information_content( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
documentation
""" | ||
return list( | ||
chain( | ||
*[ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's not use * , and ** i always find them unintuitive in the code.
_description_ | ||
""" | ||
d = defaultdict(int) | ||
for key, count in groupapply(tokens, extract_token_name_key, "count").items(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not in favor of additional dependencies - can we have a list of dependencies and discuss what's needed and what's not?
from open_rarity.models.tokens import TokenAttributeStatistic | ||
|
||
|
||
def unique_trait_count( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Naming utc clashes with utc timestamp - better naming will help us perhaps?
|
||
def __init__(self) -> None: | ||
# OpenRarity uses InformationContent as the scoring algorithm of choice. | ||
self.handler = InformationContentScoringHandler() | ||
self.handler = IC() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should be more explicit here - i like more InformationContent
This PR has two main goals...
Additionally, a breaking change that moves the "entrypoint" of the library from a scorer interface to a collection has been made.