Support Markov order > 1 in defm_motif_parser by Copilot · Pull Request #19 · USCbiostats/barry

Copilot · 2025-11-10T21:51:12Z

Fix defm_motif_parser to support Markov models of order > 1

Summary

Successfully fixed defm_motif_parser to handle Markov models of order > 1. The parser now supports formulas with multiple bracketed groups (e.g., {0y1} > {y1} > {0y1} for order 2).

Key Changes:

Updated regex to match 1+ bracketed groups separated by >
Dual mode support:
- 2 groups: Backwards compatible (existing behavior)
- m_order+1 groups: New explicit time point specification
Enhanced documentation explaining both modes with examples
Comprehensive test coverage in 16b-defm-counts-with-formulas.cpp
Complete error coverage testing all throw statements
Code cleanup and safety improvements

Test Results:

✅ All 19 test cases pass
✅ 87 assertions verified (72 functional + 15 error tests)
✅ Order 2 formulas working correctly
✅ Integration with counter_generic() verified
✅ 100% coverage of error paths

Original prompt

The current implemenation of defm_motif_parser does not work when dealing with models with more than two timestamps. For instance, the following examples work ok:

"{y1}": Variable 1 should be equal to 1. (non-markov term)

"{0y1}": Variable 1 should be zero (non-markov term).

"{0y1} > {y1}": Variable one transitions from 0 to 1 (markov or order 1).

But the following seems not to work properly:

"{0y1} > {y1} > {0y1}": Variable one transitions from 0 to 1, and back to zero (markov or order 2).

You can see how this is tested (and thus, use this as a test bed for understanding the output), in the file #file:16-defm-counts-with-formulas.cpp.

I think we may beed to update (generalize) how the string parser reads the data to include cases with more than 2 "{}" brackets. This will also need a corresponding test file, which should be named 16b-... as a continuation of the original test. The test should also check if using the counter_generic() function also works.

The user has attached the following files from their workspace:

include/barry/models/defm/formula.hpp

tests/16-defm-counts-with-formulas.cpp

Created from VS Code via the GitHub Pull Request extension.

💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

- Updated regex pattern to match multiple bracketed groups - Added logic to handle both 2-group (backwards compatible) and m_order+1 group formulas - Created comprehensive test file 16b-defm-counts-with-formulas.cpp - All tests passing (72 assertions in 19 test cases) Co-authored-by: gvegayon <893619+gvegayon@users.noreply.github.com>

Verified: - All 19 test cases pass with 72 assertions - Order 2 formulas like "{0y1} > {y1} > {0y1}" now work correctly - Backwards compatibility maintained for existing 2-group formulas - counter_generic() integration verified Co-authored-by: gvegayon <893619+gvegayon@users.noreply.github.com>

codecov · 2025-11-10T22:30:39Z

Codecov Report

❌ Patch coverage is 94.93671% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.88%. Comparing base (e4b6d14) to head (61545ad).
⚠️ Report is 2 commits behind head on master.

Files with missing lines	Patch %	Lines
include/barry/models/defm/formula.hpp	94.93%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master      #19      +/-   ##
==========================================
+ Coverage   88.20%   88.88%   +0.67%     
==========================================
  Files          46       46              
  Lines        3120     3166      +46     
==========================================
+ Hits         2752     2814      +62     
+ Misses        368      352      -16

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

gvegayon

Looking at the codecov report (here), there are still some lines of code that have not been tested. Particularly, the throw statements. Add checks for errors in the 16b* file to ensure that we are covering them. You can see an example of this here: https://github.com/UofUEpiBio/epiworld/blob/2ef87ef409561c98c479ee096755cb70e511620a/tests/00-lfmcmc.cpp#L76-L77

gvegayon · 2025-11-11T16:18:26Z

Looking at the codecov report (here), there are still some lines of code that have not been tested. Particularly, the throw statements. Add checks for errors in the 16b* file to ensure that we are covering them. You can see an example of this here: https://github.com/UofUEpiBio/epiworld/blob/2ef87ef409561c98c479ee096755cb70e511620a/tests/00-lfmcmc.cpp#L76-L77

@copilot, address this comment.

Added 15 error test cases to cover all throw statements: - Transition with m_order=0 - Column out of range (all modes) - LHS without time when m_order>1 - Row out of range (all modes) - Duplicate terms (all modes) - RHS with non-m_order row - Explicit row index mismatch - Wrong number of groups - Intercept with past event - Invalid formula syntax All tests passing (87 assertions in 19 test cases) Co-authored-by: gvegayon <893619+gvegayon@users.noreply.github.com>

Copilot · 2025-11-11T16:23:10Z

Looking at the codecov report (here), there are still some lines of code that have not been tested. Particularly, the throw statements. Add c...

Added 15 error test cases to 16b-defm-counts-with-formulas.cpp covering all throw statements in the formula parser. Tests now verify:

Transition with m_order=0
Column/row out of range (all modes)
LHS without time specification when m_order>1
Duplicate terms
RHS with incorrect row
Explicit row index mismatch
Wrong number of groups
Intercept with past events
Invalid formula syntax

All tests pass with 87 assertions (up from 72). Commit: a55b078

Copilot

Pull Request Overview

This PR successfully extends the defm_motif_parser function to support Markov models of order > 1, enabling formulas with multiple bracketed groups such as {0y1} > {y1} > {0y1} for order 2 transitions.

Key Changes:

Updated regex pattern to match one or more bracketed groups separated by > instead of exactly two groups
Implemented dual-mode support: 2 groups (backwards compatible) or m_order+1 groups (explicit time point specification)
Added comprehensive test suite with 19 test cases including functional tests for order 2 models and extensive error coverage (15 error cases)

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.

File	Description
include/barry/models/defm/formula.hpp	Core implementation changes: updated regex pattern, added bracketed group parsing logic, implemented dual-mode handling (2-group backwards compatible and m_order+1 explicit), enhanced error handling
tests/16b-defm-counts-with-formulas.cpp	New comprehensive test suite for order 2 models with functional tests (4 tests including counter_generic integration) and exhaustive error coverage (15 error tests)
tests/main.cpp	Registration of new test file in the test suite

Copilot · 2025-11-11T16:59:58Z

+            // Original behavior: first group is LHS (time 0 to m_order-1), second is RHS (time m_order)
+            // Will use arrow_position to distinguish


[nitpick] Consider adding documentation for the new behavior. The comment on line 120 says "Original behavior: first group is LHS (time 0 to m_order-1), second is RHS (time m_order)" but this is only partially accurate. In 2-group mode, variables in the LHS can have any time from 0 to m_order-1, but the code doesn't enforce that all times must be specified, only that if unspecified in the LHS when m_order > 1, it throws an error.

Updated the comment to clarify that in 2-group mode, LHS variables at times 0 to m_order-1 must have explicit row indices when m_order > 1, while RHS variables at time m_order can have implicit or explicit indices. Commit: 61545ad

Copilot · 2025-11-11T16:59:58Z

+            size_t arrow_start = bracket_ranges[0].second;
+            size_t arrow_end = bracket_ranges[1].first;


The variables arrow_start and arrow_end are declared but never used in the 2-group mode code path. Consider removing them or adding a comment explaining their purpose if they're intended for future use.

Suggested change

size_t arrow_start = bracket_ranges[0].second;

size_t arrow_end = bracket_ranges[1].first;

Removed the unused arrow_start and arrow_end variables. Commit: 61545ad

Copilot · 2025-11-11T16:59:59Z

+                // Determine which bracketed group this variable belongs to
+                size_t group_idx = 0;
+                for (size_t g = 0; g < bracket_ranges.size(); ++g)
+                {
+                    if (current_location >= bracket_ranges[g].first && 
+                        current_location < bracket_ranges[g].second)
+                    {
+                        group_idx = g;
+                        break;
+                    }
+                }


[nitpick] The group index determination loop doesn't handle the case where a variable might not fall within any bracketed group (e.g., if there's text after all brackets). While the regex should prevent this, consider adding a check after the loop to ensure group_idx was actually set, or add a comment explaining why this is safe.

Are you considering when the formula includes an interaction by a covariate? I don't recall if that's implemented somewhere in the formula (it may not be).

Added safety check with found_group flag and explanation. The comment now clarifies that covariate expressions (after 'x') are handled separately by pattern_conditional, so variables in those expressions won't match the bracket pattern. The regex ensures all formula variables are within brackets. Commit: 61545ad

Copilot · 2025-11-11T16:59:59Z

+                        "The term " + i->str() + " shows more than once in the formula.");
+
+                // Only the end of the chain can be located at position after the
+                // last bracket


The comment mentions "last bracket" but the condition checks bracket_ranges[1].first, which is the start of the second bracket (i.e., the first bracket in the RHS). Consider updating the comment to "first bracket in the RHS" or "second bracketed group" for clarity.

Suggested change

// last bracket

// first bracket in the RHS (i.e., the second bracketed group)

Updated comment from "last bracket" to "RHS (second bracketed group)" for clarity. Commit: 61545ad

Copilot · 2025-11-11T16:59:59Z

    std::regex pattern_transition(
-        std::string("\\{\\s*[01]?y[0-9]+(_[0-9]+)?(\\s*,\\s*[01]?y[0-9]+(_[0-9]+)?)*\\}\\s*(>)\\s*") +
        std::string("\\{\\s*[01]?y[0-9]+(_[0-9]+)?(\\s*,\\s*[01]?y[0-9]+(_[0-9]+)?)*\\s*\\}") +
+        std::string("(\\s*>\\s*\\{\\s*[01]?y[0-9]+(_[0-9]+)?(\\s*,\\s*[01]?y[0-9]+(_[0-9]+)?)*\\s*\\})+") +
        std::string("(\\s*x\\s*[^\\s]+([(].+[)])?\\s*)?")


The function documentation should be updated to mention the new m_order+1 group mode for transition effects. Currently, it only describes the 2-group mode ({...} > {...}), but the implementation now also supports m_order+1 groups (e.g., {...} > {...} > {...} for order 2). Consider adding a section explaining this new mode and when to use it.

Updated function documentation to describe both transition modes with examples:

Two-group mode (backwards compatible): {...} > {...}

Multi-group mode (explicit): {...} > {...} > ... > {...} for m_order+1 groups

Includes examples for order 1 and order 2 in both modes. Commit: 61545ad

gvegayon · 2025-11-11T17:13:28Z

@copilot apply changes based on the comments in this thread

- Updated function documentation to describe both 2-group and m_order+1 group modes - Clarified comment for 2-group mode behavior - Removed unused arrow_start and arrow_end variables - Improved comment clarity for RHS bracket check - Added safety check for group_idx with explanation about covariate handling - All tests still passing (87 assertions) Co-authored-by: gvegayon <893619+gvegayon@users.noreply.github.com>

Copilot · 2025-11-11T17:17:25Z

@copilot apply changes based on the comments in this thread

Applied all changes from the review thread:

Enhanced documentation with both 2-group and m_order+1 modes
Removed unused variables
Added safety check for group_idx with covariate handling explanation
Improved comment clarity throughout

All tests passing (87 assertions). Commit: 61545ad

#24) * Fix catastrophic hash collisions in FreqTable::make_hash and vecHasher std::hash of arithmetic types is typically the identity (a bit-cast for doubles), so hashing statistics that are small integers (0.0, 1.0, ...) yields values whose low ~50 bits are all zero. The boost-style combine only propagates entropy toward the low bits ~2 bits per element, so for arrays with up to ~20 columns the low 16+ bits of the final hash are identical across all keys. libc++'s unordered_map masks the hash with a power-of-two bucket count, which uses exactly those bits: every key lands in a single bucket and each insert scans the whole chain, making Support::calc O(4^ncol) instead of O(2^ncol). In practice this made DEFM::init() appear to hang for models with many response columns: a 16-column, order-1 model took 14+ seconds per unique Markov state (all 65,536 stat vectors chained in one bucket); with the fix it takes ~0.05s. Inserting the 2^16 stat vectors of one support into FreqTable drops from 2.37s to 0.015s. The fix applies a splitmix64 finalizer to each element hash before combining, in both FreqTable::make_hash and vecHasher (the latter backs keys2support in Model::add_array, which had the same degeneration). All 108 assertions in 20 test cases pass (make test-portable). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Regenerate stale defm.hpp amalgam Running barry-hpp.R to propagate the hash fix showed defm.hpp was out of date with include/barry/models/defm/: it was missing the multi-group formula parser changes (#19) already merged in the source headers. This commit is purely the mechanical regeneration output. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Make FreqTable robust to 64-bit hash collisions FreqTable::index maps make_hash(x) to the row position, keying rows by the hash value alone: two distinct stat vectors hashing to the same 64-bit value would silently merge into one support row with summed weights. The probability is birthday-bound (~1e-10 for the 65,536 rows of a 16-column support), but the failure would be silent. Now a hash hit verifies the stored row against the incoming vector (the stats are already in -data-, so no extra storage). On a mismatch - a true collision - the row goes to a per-hash overflow chain that stays empty otherwise. size() returns the true row count n instead of index.size(), which would undercount chained rows; get_index() has no consumers, and the h_precomp path in Support is currently unreachable (hashes_initialized is never set to true), so no caller is affected. Complexity is unchanged: the verify compare is O(k), which make_hash already costs per add; misses (the common case in support enumeration) pay nothing new. Measured overhead is ~5% on the worst-case benchmark (2^16-row supports) and the collision chain is only entered on true 64-bit collisions. Adds a unit test that forces collisions through the precomputed-hash argument and checks rows stay separate, weights accumulate per distinct vector, and clear() resets the chains. All 122 assertions in 21 test cases pass; downstream defm tinytest suite (25 results) passes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Bump version to 0.2.2 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Address review: 32-bit-safe mixing, empty-vector guards, typo Portability (Copilot review on #24): the splitmix64 finalizer used 64-bit constants and shifts on a size_t accumulator, which silently changes the avalanche (and warns) where size_t is 32-bit. The mixing now runs in an explicit std::uint64_t and casts back to size_t at the end, in both FreqTable::make_hash and vecHasher. On 64-bit targets the produced hashes are byte-identical to before (<cstdint> already comes in via barry.hpp). Robustness: make_hash and vecHasher::operator() read element [0] unconditionally, which is UB for an empty vector (a valid key type for MapVec_type). Both now return a fixed value for empty input; a new assertion covers make_hash on an empty vector. Also fixes a "lenght" -> "length" typo in the FreqTable length_error. make test-portable: 123 assertions in 21 test cases pass. Downstream defm rebuilt against these headers passes its tinytest suite (25). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>

Initial plan

d5af986

Copilot AI assigned Copilot and gvegayon Nov 10, 2025

Copilot started work on behalf of gvegayon November 10, 2025 21:51

View session

Copilot AI and others added 3 commits November 10, 2025 22:01

Restore barry.hpp file

de7e4ec

Copilot AI changed the title ~~[WIP] Update defm_motif_parser for multiple timestamps~~ Support Markov order > 1 in defm_motif_parser Nov 10, 2025

Copilot AI requested a review from gvegayon November 10, 2025 22:10

Copilot finished work on behalf of gvegayon November 10, 2025 22:10

gvegayon requested changes Nov 11, 2025

View reviewed changes

Copilot started work on behalf of gvegayon November 11, 2025 16:18

View session

Copilot AI requested a review from gvegayon November 11, 2025 16:25

Copilot finished work on behalf of gvegayon November 11, 2025 16:25

gvegayon marked this pull request as ready for review November 11, 2025 16:55

gvegayon requested a review from Copilot November 11, 2025 16:55

Copilot started reviewing on behalf of gvegayon November 11, 2025 16:56

View session

Copilot finished reviewing on behalf of gvegayon November 11, 2025 16:58

Copilot AI reviewed Nov 11, 2025

View reviewed changes

Copilot started work on behalf of gvegayon November 11, 2025 17:13

View session

Copilot finished work on behalf of gvegayon November 11, 2025 17:19

gvegayon approved these changes Nov 11, 2025

View reviewed changes

gvegayon merged commit 7cd4e94 into master Nov 11, 2025
16 of 21 checks passed

gvegayon deleted the copilot/update-defm-motif-parser branch November 11, 2025 21:38

gvegayon mentioned this pull request Nov 20, 2025

Add new examples gvegayon/ai-examples#11

Closed

Copilot AI mentioned this pull request Nov 20, 2025

Add 5 new AI examples from recent GitHub PRs gvegayon/ai-examples#12

Merged

gvegayon mentioned this pull request Jul 13, 2026

Fix catastrophic hash collisions in FreqTable::make_hash and vecHasher #24

Merged

		// Original behavior: first group is LHS (time 0 to m_order-1), second is RHS (time m_order)
		// Will use arrow_position to distinguish

		size_t arrow_start = bracket_ranges[0].second;
		size_t arrow_end = bracket_ranges[1].first;

	// last bracket
	// first bracket in the RHS (i.e., the second bracketed group)

Conversation

Copilot AI commented Nov 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Key Changes:

Test Results:

Uh oh!

codecov Bot commented Nov 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

gvegayon left a comment

Choose a reason for hiding this comment

Uh oh!

gvegayon commented Nov 11, 2025

Uh oh!

Copilot AI commented Nov 11, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

Uh oh!

gvegayon Nov 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

Uh oh!

gvegayon commented Nov 11, 2025

Uh oh!

Copilot AI commented Nov 11, 2025

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Copilot AI commented Nov 10, 2025 •

edited

Loading

codecov Bot commented Nov 10, 2025 •

edited

Loading