Conversation
src/verifai/features/features.py
Outdated
| if length is None: | ||
| domain.flattenOnto(value, flattened) | ||
| else: | ||
| fixedDomain = feature.fixedDomains(self.timeBound)[length] | ||
| fixedDomain.flattenOnto(value, flattened) | ||
| if fixedDimension: | ||
| sizePerElt = domain.flattenedDimension | ||
| needed = (feature.maxLength - length) * sizePerElt | ||
| for i in range(needed): | ||
| flattened.append(None) |
There was a problem hiding this comment.
Can we pull this code out into a helper function (can be local, doesn't need to be a method on the class) and use it in the loop for static features too? Most of the code is identical.
There was a problem hiding this comment.
(We can leave this to later, it's not that important)
| dim += 1 # Timesteps | ||
| for feature in self.features: | ||
| domain = feature.domain | ||
| timeMult = self.timeBound if isinstance(feature, TimeSeriesFeature) else 1 |
There was a problem hiding this comment.
A thought probably best left for later: maybe we should move all the flattening logic into new Feature._flatten and Feature._flattenedDimension methods. Then this kind of case split could be handled by overrides in TimeSeriesFeature instead of needing to have special cases here.
dfremont
left a comment
dfremont
left a comment
There was a problem hiding this comment.
Looks good overall. Details are in comments above, but my main suggestion is to try to preserve the simple way to access features from the sampled point (point.myFeature instead of point.staticSample.myFeature). If we can do that, then most of the changes you had to make to the existing tests shouldn't be necessary.
Co-authored-by: Daniel Fremont <dfremont@ucsc.edu>
2 participants
Adds dynamic sampling to VerifAI.
Merge with BerkeleyLearnVerify/Scenic#417