Release/1.0.0 #94
Release/1.0.0 #94
Conversation
openrarity/collection/collection.py
Outdated
|
|
||
|
|
||
| class TokenCollection: | ||
| """ """ |
There was a problem hiding this comment.
Just a placeholder. Needs the docstring completed
|
|
||
| def __init__( | ||
| self, | ||
| token_type: Literal["non-fungible", "semi-fungible"], |
There was a problem hiding this comment.
Perhaps should alias it as a type?
| "metric.information", | ||
| "metric.information_entropy", | ||
| "metric.unique_trait_count", | ||
| "metric.max_trait_information", |
There was a problem hiding this comment.
What is max_trait_information? Also suggest to make it a type with enums
There was a problem hiding this comment.
Is the maximum information content on a single trait for a token. It doesn't have a fixed value so Enums wouldn't apply here.
|
|
||
| @property | ||
| def entropy(self) -> float | None: | ||
| return self._entropy |
There was a problem hiding this comment.
entropy is also computed and should follow the same assignment pattern in the code, no?
There was a problem hiding this comment.
In this self._entropy is the calculated value and using @property to decorate a method like this effectively creates a read only variable.
| "metric.max_trait_information", | ||
| ], | ||
| ..., | ||
| ] = ("metric.unique_trait_count", "metric.information_entropy"), |
There was a problem hiding this comment.
that assignment looks a bit cryptic to me. Should We simplify this by bringing the type to some predefined static variable something like OR_SORT_FUNCTION =("metric.unique_trait_count", "metric.information_entropy")
There was a problem hiding this comment.
I am indifferent. Its the openrarity library and a default value was provided so it can be assumed that it is the OR preferred one. Masking it with a static variable will mess up introspection I believe.
| return self._input_checksum + self._ranks_checksum | ||
|
|
||
| @overload | ||
| def rank_collection( # type: ignore |
There was a problem hiding this comment.
need better documentation in methods , otherwsie not clear why those overloads exists and why we need them
| "metric.unique_trait_count", | ||
| "metric.max_trait_information", | ||
| ], | ||
| ..., |
There was a problem hiding this comment.
Not familiar with ... syntax, what is it doing?
There was a problem hiding this comment.
The ... in tuple[] just means that it can be of arbitrary length. So specifically the above typing says that it is a tuple with any number of the above literal values.
openrarity/collection/collection.py
Outdated
| (self._token_supply, self._tokens) = validate_tokens(token_type, tokens) | ||
|
|
||
| # Derived data | ||
| self._vertical_attribute_data: list[ValidatedTokenAttribute] = [] |
There was a problem hiding this comment.
what is vertical_attribute_data? Not clear naming suggest to rename
openrarity/io/read.py
Outdated
| return json.loads(Path(path).read_text()) | ||
|
|
||
|
|
||
| def from_csv(path: str | Path) -> dict[str | int, dict[Any, Any]]: |
There was a problem hiding this comment.
perhaps remove it? why keep not implemented method?
There was a problem hiding this comment.
Was thinking it would be implemented eventually but honestly now I really don't intend to.
openrarity/metrics/ic.py
Outdated
| Parameters | ||
| ---------- | ||
| counts : list[AttributeStatistic] | ||
| _description_ |
| AttributeStatistic, | ||
| { | ||
| **attr, | ||
| "metric.probability": min(1, attr["attribute.supply"] / total), |
There was a problem hiding this comment.
how it can be >1, if it's >1 it's more like a bug no?
There was a problem hiding this comment.
numeric precision
| { | ||
| **attr, | ||
| "metric.probability": min(1, attr["attribute.supply"] / total), | ||
| "metric.information": max(0, -log2(attr["attribute.supply"] / total)), |
There was a problem hiding this comment.
same comment here , not sure if we should be defensive in code here.
There was a problem hiding this comment.
again numeric precision
| list[AttributeStatistic] | ||
| Augmented Tokens | ||
| """ | ||
| return [] |
openrarity/metrics/trait_count.py
Outdated
| def count_traits( | ||
| attributes: list[MetadataAttribute], | ||
| ) -> list[MetadataAttribute]: | ||
| """Count the number of traits on a given token and append it as an attribute named |
There was a problem hiding this comment.
not sure i understand the logic , how it's working ? in the code is can see how we append something to an array.
There was a problem hiding this comment.
The code uses list comprehension to generate the array with the token and its count. Documentation can be better though.
In python doing
lst = []
for val in something:
lst.append(val)is significantly less performant than list comprehensions. List comprehensions are very pythonic so I am going to fight back against removing them.
List comp
lst = [val for val in something]
openrarity/utils/aio.py
Outdated
| from tqdm.asyncio import tqdm_asyncio # type: ignore | ||
|
|
||
|
|
||
| async def semaphore_gather(num: int, coros: Coroutine[Any, Any, Any]) -> list[Any]: |
There was a problem hiding this comment.
Perhaps small doc on utility of this asyncIO tooling will be helpful
| pandas = "^1.4.3" | ||
| scipy = "^1.9.0" | ||
| requests = "^2.28.1" | ||
| aiolimiter = "^1.0.0" |
There was a problem hiding this comment.
i'd like us to keep depdency footprint limited. Let's discuss in chat what each new dependency brings.
| @@ -0,0 +1,555 @@ | |||
| # import argparse | |||
There was a problem hiding this comment.
commented code need to remove it or keep it?
release_1.0.0 branch initial enhancements
3 participants
No description provided.