-
Notifications
You must be signed in to change notification settings - Fork 1
More general implementation #21
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
base: main
Are you sure you want to change the base?
Conversation
In my view, this is now ready. I'm happy about any and all feedback/comments! There's an accompanying PR to update QuanticsTCI.jl at tensor4all/QuanticsTCI.jl#14. |
Nice work! One question: Wouldn't it be better to infer the I have some small cleanup I'll propose in a separate commit |
We are traveling now. Let us take some time. |
Thanks!
The problem then is that you don't know the correct order of the dimensions, which is fine if you only call functions specifying the variable names (like
Perfect, looking forward to it! |
Thank you for the useful feature! I tried this branch, and the tests passed locally. I noticed that the documentation for DiscretizedGrid{D} is listed under API Reference. |
OK, I finally got around to looking at everything, and it's great! I think one place where this can be improved is the handling of invalid variable / index tables, see
Currently only one of these actually throws, with a non-specific AssertionError .
I'm also a bit afraid that it may be possible to bypass some of these parameter consistency checks, as they are only defined in some constructors, not others. In my opinion, the correct place to define them is in the place where the argument is actually used. For example, the consistency of the index table should probably be checked in
Other than that, the code is pretty good, and we're close to convergence! PS: I think for the file structure you can just put your tests in |
As discussed with Marc, here's a complete rewrite of the
DiscretizedGrid
type, tentatively calledNewDiscretizedGrid
.Description & Interface
Everything not described in the docstring, which I just pasted below, (e.g. the conversion functions) works exactly as in the existing implementation.
It is intended to be usable as a drop-in replacement for the existing
DiscretizedGrid
type except for some small differences in behavior, e.g. regardingupper_bound
(which is shifted depending onincludeendpoint
so we don't need to storeincludeendpoint
explicitly). It also doesn't subtypeGrid{D}
at the moment.Performance
The new implementation includes a special path for the common
base = 2
case using bit operations, so it is slightly faster.Here's the results of a benchmark script, measuring the performance of quantics <-> origcoord conversion and comparing with
DiscretizedGrid
:Benchmark script
This needs Chairmarks.jl installed.