Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #244 +/- ##
==========================================
+ Coverage 82.66% 83.01% +0.34%
==========================================
Files 43 44 +1
Lines 5583 5621 +38
==========================================
+ Hits 4615 4666 +51
+ Misses 968 955 -13 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
lkdvos
left a comment
There was a problem hiding this comment.
I really like this change a lot!
I left some comment about being able to display the current cache usage, and additionally I was wondering if the caching could be something worth creating a small library for, since I would like to have the option to support caching of the Fsymbol and Rsymbols in TensorKitSectors as well. We don't necessarily have to share the same machinery for that, since there is also the question of possibly storing these on disk, but I did want to bring this up to discuss here.
|
These are all good suggestions, but I first want to understand how this change can end up breaking LRUCache. These errors come straight out of the LRUCache functions which should have locked the cache, so I don't know how that is possible, and why it only happens in the case of Regarding making this into a separate package; I am not sure if I can maintain more general-purpose packages :-). Also, if we end up using |
|
@lkdvos, thanks for finding the bug and fixing it. Not sure why CI didn't run on your last commit? |
|
Oh I see, it is still waiting for the CI of the other PR to finish. |
|
I have absolutely no idea about the CI, I had to trigger that manually somehow |
lkdvos
left a comment
There was a problem hiding this comment.
I think that this would actually be a nice addition, so perhaps we can push this through?
|
@Jutho friendly reminder that this PR is more or less ready, so it might be reasonable to try and push it over the edge |
|
So as a compromise, I set the default cache size to |
Too bad real-life compromises are typically not made on a logarithmic scale 😄. Anyway, I like the latest changes to this PR. |
|
I'll try and run the benchmarks before merging, and post the results here. |
|
Overall, this looks like nothing really changed within the margins of this limited benchmarksuite, so good to merge for me! Benchmark results
|
A first attempt at resolving #242