Reintroduce AbstractNetworkIterator and AbstractProblem interface
#17
Conversation
|
I am going to look into expressing this interface in more general language. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17 +/- ##
==========================================
+ Coverage 56.51% 58.88% +2.37%
==========================================
Files 6 8 +2
Lines 476 574 +98
==========================================
+ Hits 269 338 +69
- Misses 207 236 +29
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Co-authored-by: Matt Fishman <mtfishman@users.noreply.github.com>
Co-authored-by: Matt Fishman <mtfishman@users.noreply.github.com>
src/iterators.jl
Outdated
| try | ||
| first(region_plan) | ||
| catch e | ||
| if e isa BoundsError | ||
| throw(ArgumentError("Cannot construct a region iterator with 0 elements.")) | ||
| end | ||
| rethrow() | ||
| end |
There was a problem hiding this comment.
I'm always a bit weary to use try except in extenuating circumstances, when is it not enough to use something like isempty(region_plan)? It seems reasonable to expect whatever is input as region_plan to define some functions like isempty, length, etc.
There was a problem hiding this comment.
Also, is it really a problem to have an empty RegionIterator? Can't that just mean that that if you iterate it it doesn't do anything?
There was a problem hiding this comment.
I like try as it allows one to both print a more descriptive error higher in the callstack as well as the error that would be inevitably be thrown later, but Ill be honest I don't have that much formal programming knowledge of how the statements should be used, so happy to use some other pattern if you think it would be more suitable.
Constructing empty AbstractNetworkIterators is undefined as the first iteration is implicit. This is a consequence of having both of the following behaviors:
- Work done during iteration. i.e. allowing
for _ in iter endto do the full computation - An
AbstractNetworkIteratorhaving a well defined state during the callback.
Might be easier to discuss this in person if you are uncomfortable with disallowing empty AbstractNetworkIterator, but I believe it to be fundamental to allow for the above two behaviors (that we already agreed were good behaviors).
There was a problem hiding this comment.
Yes I think this would be easier to discuss in person, I've gotten a little bit lost around those edge cases in the design. If you want to write a custom error message I don't see why something closer to the previous design doesn't suffice, i.e.:
if isempty(region_plan)
throw(ArgumentError("Cannot construct a region iterator with 0 elements."))
endbut maybe I'm missing something.
There was a problem hiding this comment.
Is the issue that isempty(region_plan) might sometimes throw an error? If so, in which cases would it throw an error?
There was a problem hiding this comment.
You are not missing anything, I am overcomplicating things!
There was a problem hiding this comment.
If the concern is that region_plan might be an iterator that doesn't have a length/size defined, maybe we can catch that more explicitly by checking Base.IteratorSize(region_plan) == Base.SizeUnknown() (https://docs.julialang.org/en/v1/manual/interfaces/#man-interface-iteration). (But also that seems like a strange corner case that we can deal with if it comes up.)
… `region_plan` Now just throws an `ArgumentError`
|
@jack-dunham this looks good from my end, is there anything else you wanted to do here? Also can you bump the package version? |
Happy to add in the contents of |
Sure, we can include that here. |
2 participants
See discussions in: ITensor/ITensorNetworks.jl#255