algebra
algebra copied to clipboard
EvaluationDomain::reindex_by_subdomain ergonomics and doc improvements
Here's the method signature:
https://github.com/arkworks-rs/algebra/blob/273bf2130420904cab815544664a539f049d0494/poly/src/domain/mod.rs#L284-L287
Wish list
- The caller must pass ownership of
other
. Thus, anyone who wants to use this method must cloneother
, which seems wasteful. Instead the type ofother
should be&Self
. But this item is obsoleted by the next item... - The only use of
other
in this implementation is to callother.size()
, which merely returns ausize
struct field. Thus, there is no need for an already-constructedEvaluationDomain
forother
here. Indeed, I suspect a common use of this method is where a caller knows the size ofother
but hasn't actually constructed aEvaluationDomain
for it. In those use cases this method forces the caller to construct anEvaluationDomain
just to call this method. I suggest changing the type ofother
fromSelf
tousize
. In that case it's name should be changed to something likeother_domain_size
. - Code comments are hard for me to understand. Seems like the rustdoc for this method should read something like
/// Convert `index` from [an index into an element from `other`] into [an index of an element from `self`].
///
/// `other` must be a sub-domain of `self`.
/// etc.
- The following comment https://github.com/arkworks-rs/algebra/blob/273bf2130420904cab815544664a539f049d0494/poly/src/domain/mod.rs#L289-L292 can be replaced with something shorter:
// `other` is a subgroup of `self`. Thus, index `i` in `other` is index `i*|self|/|other|` in `self.
- I don't understand the following comment
https://github.com/arkworks-rs/algebra/blob/273bf2130420904cab815544664a539f049d0494/poly/src/domain/mod.rs#L297-L304
It's not clear to me why this method should allow
index >= other.size()
. Shouldn't this be an out-of-bounds error?