Descriptor rework, counterexamples, and bug fixes #7
Conversation
…nd a bug in injective DTW normalization.
…se an unclamped variant or not.
…ations of descriptor behavior.
facebook-github-bot
added
the
CLA Signed
syntheticmagus
changed the title
| auto ets = EgocentricTemporalSpace::getPose(priorWristPosition, priorHmdPose); | ||
| auto ets = EgocentricTemporalSpace::getPose(wristPosition, hmdPose); |
There was a problem hiding this comment.
This change switches the egocentric temporal space from being that of the prior frame to that of the current frame. This prevents this descriptor from being framerate-dependent.
…tation got there.
| if (currentRow[idx] < minimum) | ||
| size_t connectionsCount = std::max(idx, b.size()); | ||
| if (currentRow[idx] / connectionsCount < minimum) | ||
| { | ||
| minimumIdx = idx; | ||
| minimum = currentRow[minimumIdx]; | ||
| minimum = currentRow[minimumIdx] / connectionsCount; |
There was a problem hiding this comment.
The bug fixed by this --- that injective distance, with its inherent ability to accept matches with varying numbers of pairings, will overwhelmingly bias toward short projections unless normalization is considered --- had been affecting CARL since injective DTW was introduced at the very beginning.
| static_cast<size_t>( | ||
| (original.getEndTimestamp() - original.getStartTimestamp()) / frameDuration), | ||
| 1); | ||
| size_t newSamplesCount = std::max<size_t>(static_cast<size_t>(std::ceil((endTimestamp - startTimestamp) / frameDuration)), 1); |
There was a problem hiding this comment.
It's subtle, but the inclusion of std::ceil in this equation is also a significant bug fix. Without that, the last sample of a resampling will typically be from before endTimestamp, which in very short actions at slow framerates can cause the final pose of the action to be "sampled out." By adding ceil, an additionally sample is typically introduced, ensuring that the final pose of the example is represented in the resampling.
| double t = (timestamp - original.getStartTimestamp()) / | ||
| (original.getEndTimestamp() - original.getStartTimestamp()); |
There was a problem hiding this comment.
This is the kind of bug that makes me question whether the bug knows something I no longer remember. It's unsurprising that this wouldn't have caused much of a problem since it would have simply done the wrong interpolation between two sequential samples, which should vary timing by only a very tiny amount. However, with the introduction of std::ceil mentioned above, timestamp can sometimes have values larger than endTimestamp, which could theoretically extrapolate samples that were never implied by the original recording.
| @@ -134,104 +143,136 @@ namespace carl::action | |||
| using SignalT = Signal<gsl::span<const InputSample>>; | |||
There was a problem hiding this comment.
Quite a few bug fixes here; this method was basically rewritten because it didn't work at all before, which makes sense as it was all but untested without a usable integration.
| m_tuning[idx] = 1;/*t * DescriptorT::DEFAULT_TUNING[idx] + | ||
| (1. - t) * (1. / std::max(averageDistances[idx], kAverageDistanceEpsilon));*/ |
There was a problem hiding this comment.
Auto-tuning is essentially disabled at the moment. This situation will not last, but it remains to be explored whether tuning should be adapted to a new meaning ("leniency" or something like that) or eliminated entirely.
Significant rework of descriptors, changing the distance arithmetic to have consistent practices across descriptors. The also brings in support for counterexamples, numerous bug fixes, and implementation changes/cleanup affecting issues including improved resampling and better support for framerate independence.