Define variants for repeats#1633
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces repeat variants (is_repeat_parent, is_repeat_child, and repeat_index) to Ramble's application base class, registering them as reserved variants and adding a test to verify their presence during analysis. Feedback on these changes highlights an issue where repeat variants are defined without being expanded or typed, and the variant cache is not cleared when the variants dictionary is empty. A suggestion was provided to expand these variables and conditionally clear the cache to prevent stale data.
| # Define variants from repeats | ||
| for keyword in [ | ||
| self.keywords.is_repeat_parent, | ||
| self.keywords.is_repeat_child, | ||
| self.keywords.repeat_index, | ||
| ]: | ||
| if keyword in variables: | ||
| self.object_variants.experiment_variant( | ||
| keyword, variables[keyword] | ||
| ) | ||
|
|
There was a problem hiding this comment.
When defining variants from repeats, the values from variables are not expanded or typed, and the application's variant cache is not cleared if the variants dictionary is empty.
- Value Expansion: If any of the repeat variables contain template references or are defined as strings (e.g., "True"/"False"), they should be expanded and typed using
self.expander.expand_var(..., typed=True)to ensure consistency with other experiment variants. - Cache Invalidation: If
variantsis empty, the blockif variants:is skipped, meaningself.clear_variant_cache()is never called. This leaves the application with a stale variant cache, which can cause subsequent calls (likeself._set_system()) to not see the newly defined repeat variants.
We should expand the values and conditionally clear the variant cache if any repeat variants were defined.
# Define variants from repeats
has_repeat_variants = False
for keyword in [
self.keywords.is_repeat_parent,
self.keywords.is_repeat_child,
self.keywords.repeat_index,
]:
if keyword in variables:
self.object_variants.experiment_variant(
keyword, self.expander.expand_var(variables[keyword], typed=True)
)
has_repeat_variants = True
if has_repeat_variants:
self.clear_variant_cache()
Ramble Performance Test MetricsResults produced with commit: 98b7e8b
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #1633 +/- ##
========================================
Coverage 93.19% 93.19%
========================================
Files 345 345
Lines 33456 33476 +20
========================================
+ Hits 31178 31198 +20
Misses 2278 2278 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This commit adds definitions for three new variants related to repeats. - is_repeat_parent - is_repeat_child - repeat_index These can then be used by objects to conditionally apply definitions based on if the experiment is a repeat or not.
ae6201d to
98b7e8b
Compare
This merge adds definitions for three new variants related to repeats.
These can then be used by objects to conditionally apply definitions based on if the experiment is a repeat or not.