Conversation
| 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.
Co-authored-by: Daniel Fremont <dfremont@ucsc.edu>
| if attr in space.staticFeatureNamed: | ||
| return getattr(self.staticSample, attr) | ||
| elif attr in space.dynamicFeatureNamed: | ||
| class DynamicFeatureHelper: |
There was a problem hiding this comment.
Any reason not to also implement __len__ and inherit from collections.abc.Sequence?
|
|
||
| ### Feature spaces | ||
| class TimeSeriesFeature(Feature): | ||
| """A feature with a value at each timesetep of a simulation.""" |
There was a problem hiding this comment.
| """A feature with a value at each timesetep of a simulation.""" | |
| """A feature with a value at each timestep of a simulation.""" |
| return self.last_sample | ||
| @abstractmethod | ||
| def getSample(self): | ||
| """Returns a `Sample` object""" |
There was a problem hiding this comment.
| """Returns a `Sample` object""" | |
| """Generate a `Sample`.""" |
|
|
||
| FeatureSampler works as follows: | ||
| 1. Sample lengths of feature lists. | ||
| 2. Expand TimeSeriesFeatures into flattened features with of length |
There was a problem hiding this comment.
| 2. Expand TimeSeriesFeatures into flattened features with of length | |
| 2. Expand TimeSeriesFeatures into flattened features of length |
| 1. Sample lengths of feature lists. | ||
| 2. Expand TimeSeriesFeatures into flattened features with of length | ||
| space.timeBound. | ||
| 2. Sample from the resulting fixed-dimensional Domains. |
There was a problem hiding this comment.
| 2. Sample from the resulting fixed-dimensional Domains. | |
| 3. Sample from the resulting fixed-dimensional Domains. |
|
|
||
| sample = CompleteSample(space=self, staticSample=staticSample, dynamicSampleList=dynamicSampleList, updateCallback=updateCallback,dynamicSampleLengths=dynamicSampleLengths) | ||
|
|
||
| for _ in range(duration): |
There was a problem hiding this comment.
Replace with unroll method? @dfremont feel free to suggest a different name.
| return super().__getattr__(attr) | ||
|
|
||
|
|
||
| class CompleteSample(Sample): |
There was a problem hiding this comment.
This is an implementation detail, so let's make this class private. Users should only need to know about the Sample API.
| for _ in range(duration): | ||
| sample.getDynamicSample() |
There was a problem hiding this comment.
As we discussed, this is probably better handled by an option to CompleteSample.
| lists, then sampling from the resulting fixed-dimensional Domain. | ||
| """ FeatureSampler that greedily samples a CompleteSample. | ||
|
|
||
| FeatureSampler works as follows: |
There was a problem hiding this comment.
| FeatureSampler works as follows: | |
| LateFeatureSampler works as follows: |
| class LateFeatureSampler(FeatureSampler): | ||
| """FeatureSampler that works by first sampling only lengths of feature | ||
| lists, then sampling from the resulting fixed-dimensional Domain. | ||
| """ FeatureSampler that greedily samples a CompleteSample. |
There was a problem hiding this comment.
Need to reword this in a clearer way that doesn't refer to the private CompleteSample.
| static_features = [v for v in domainPoint._asdict().items() | ||
| if v[0] in self.space.staticFeatureNamed] | ||
| dynamic_features = [v for v in domainPoint._asdict().items() | ||
| if v[0] not in self.space.staticFeatureNamed] |
There was a problem hiding this comment.
This seems like a lot of work to be doing every sample. Can't we precompute the static and dynamic feature names? Alternatively, maybe it would simplify the code to split the domain into two domains, one for the static features and one for the dynamic features (i.e. FeatureSpace.staticDomains and FeatureSpace.dynamicDomains).
| static_point = self.space.makeStaticPoint(*[v[1] for v in static_features]) | ||
|
|
||
| dynamic_points = [] | ||
| if any(isinstance(f, TimeSeriesFeature) for f in self.space.features): |
There was a problem hiding this comment.
| if any(isinstance(f, TimeSeriesFeature) for f in self.space.features): | |
| if self.space.hasTimeSeries: |
| dynamic_points = [] | ||
| if any(isinstance(f, TimeSeriesFeature) for f in self.space.features): | ||
| for t in range(self.space.timeBound): | ||
| point_dict = {} |
There was a problem hiding this comment.
Can't you just use a list rather than a dict?
| if "params" not in kwargs: | ||
| kwargs["params"] = {} | ||
|
|
||
| kwargs["params"]["timeBound"] = maxSteps if maxSteps else 0 |
There was a problem hiding this comment.
| if "params" not in kwargs: | |
| kwargs["params"] = {} | |
| kwargs["params"]["timeBound"] = maxSteps if maxSteps else 0 | |
| params = kwargs.setdefault("params", {}) | |
| params["timeBound"] = maxSteps if maxSteps else 0 |
Also, arguably if maxSteps is not specified here but the Scenic program defines timeBound (which will need to be documented in the Scenic docs, in the External Parameters section), we should use the value in the program. For that you could use maxSteps or params.get("timeBound", 0).
| def fromScenicCode(cls, code, maxIterations=None, maxSteps=None, | ||
| ignoredProperties=None, **kwargs): | ||
| """As above, but given a Scenic program as a string.""" | ||
| if "params" not in kwargs: |
| """ | ||
|
|
||
| def test_double_time_series_access(): | ||
| with pytest.raises(RuntimeError): |
There was a problem hiding this comment.
This is rather broad. Can you check for a specific exception message?
| @@ -0,0 +1,19 @@ | |||
| param map = localPath('Town01.xodr') | |||
| param carla_map = 'Town01' | |||
There was a problem hiding this comment.
Not needed, since this is running in Newtonian.
| param map = localPath('Town01.xodr') | ||
| param carla_map = 'Town01' | ||
|
|
||
| model scenic.domains.driving.model |
There was a problem hiding this comment.
Why not just specify Newtonian here and then you don't need to when compiling the scenario?
| sample = sampler.nextSample() | ||
| sample = sampler.getSample() | ||
| sample.update(None) | ||
| sample = sample |
| sample = sampler.nextSample() | ||
| sample = sampler.getSample() | ||
| sample.update(None) | ||
| sample = sample |
| }) | ||
| sampler = FeatureSampler.gridSamplerFor(space) | ||
| samples = list(sampler) | ||
| samples = [s for s in sampler] |
There was a problem hiding this comment.
Why this change? It's equivalent, right?
| 'c': TimeSeriesFeature(Box((2,5))), | ||
| 'd': TimeSeriesFeature(Box((5,6)), lengthDomain=DiscreteBox((0,2))) | ||
| }, | ||
| timeBound=10) |
There was a problem hiding this comment.
Indentation here is confusing.
| check([sampler.nextSample() for i in range(100)]) | ||
| check(list(itertools.islice(sampler, 100))) | ||
| check([sampler.getSample() for i in range(100)]) | ||
| check(list(s for s in itertools.islice(sampler, 100))) |
There was a problem hiding this comment.
Redundant comprehension.
2 participants
Adds dynamic sampling to VerifAI.
Merge with BerkeleyLearnVerify/Scenic#417