-
Notifications
You must be signed in to change notification settings - Fork 36
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
Potential improvements for _SearchIndexer #741
Comments
These are good ideas. Since all this is internal, this work can be done post-2.0. |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
FWIW just tried running the test suite with the normalization removed, and everything passed. I also tried running it throwing an error whenever the normalization changed the input, and the only difference I observed was a conversion from tuples to lists. In terms of performance, the normalization costs a few microseconds for a trivial filter (e.g. |
#683 replaced the old
Collection
class with a stripped down_SearchIndexer
that does just enough for signac's internal use cases. However, that PR left a few tasks outstanding that we should consider:_SearchIndexer
with a plain dict and free functions. I don't anticipate that these changes will have a significant effect on the code base, but it's worth considering since it may improve clarity. The_SearchIndexer
has essentially no state beyond that of a normal dict, and while its contents are more restricted there is no particular enforcement, so conceptually using a free function might be clearer. Additionally, it is possible to create the object in an invalid state with the current approach. Some additional discussion is here._SearchIndexer
. This is the lowest priority task and would be moot if we reimplemented it as a dict with free functions.The text was updated successfully, but these errors were encountered: