Repository navigation
Add test coverage for counter name generation with Markov order > 1 - #20
Conversation
Co-authored-by: gvegayon <893619+gvegayon@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #20 +/- ##
=======================================
Coverage 88.88% 88.88%
=======================================
Files 46 46
Lines 3166 3166
=======================================
Hits 2814 2814
Misses 352 352 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| ); | ||
|
|
||
| model3.init(); | ||
| auto names3 = model3.get_counters()->get_names(); |
There was a problem hiding this comment.
Running this locally, I can see that the generated name is not correct. I was expecting:
"Motif {A-}⇨{A+}⇨{A-}"But I get:
"Motif {A-A+}⇨{A-}"There is an issue with how the function counter_formula (or another one called deeper) generates the names.
There was a problem hiding this comment.
Fixed in commit 9974afb. The name generation now properly shows each time point separately: "Motif {A-}⇨{A+}⇨{A-}" as expected.
| REQUIRE(names3.size() == 1); | ||
| // Should still produce the same name as counter_generic | ||
| // because both represent the same motif | ||
| REQUIRE(names3[0] == names1[0]); |
There was a problem hiding this comment.
This is fine, but both are generating the wrong name. See comment I made on line 116.
There was a problem hiding this comment.
Fixed in commit 9974afb. Both counter_generic and counter_formula now generate the correct name format with separate time points.
| auto names4 = model4.get_counters()->get_names(); | ||
|
|
||
| #ifndef CATCH_CONFIG_MAIN | ||
| printf("Test 4 (counter_formula, 3-group, multi var): '%s'\n", names4[0].c_str()); | ||
| #endif | ||
|
|
||
| #ifdef CATCH_CONFIG_MAIN | ||
| REQUIRE(names4.size() == 1); | ||
| // Should contain both A and C (custom names for y0 and y2) | ||
| REQUIRE(names4[0].find("A") != std::string::npos); | ||
| REQUIRE(names4[0].find("C") != std::string::npos); | ||
| REQUIRE(names4[0].find("Motif") != std::string::npos); |
There was a problem hiding this comment.
I see the same issue here. This is the name that is generated:
"Motif {A+, C+A+, C-}⇨{A-, C+}"But it should be
"Motif {A+, C+}⇨{A+, C-}⇨{A-, C+}"There was a problem hiding this comment.
Fixed in commit 9974afb. The name is now correctly generated as "Motif {A+, C+}⇨{A+, C-}⇨{A-, C+}" with all three time points shown separately.
…er > 1 Co-authored-by: gvegayon <893619+gvegayon@users.noreply.github.com>
|
OK, so after I made this change, the CI is not working. @copilot, fix the issue. Beyond reverting the change, I wanted to simplify the notation. I don't like using UTF-8 or special LaTeX characters, instead, I want to use the |
Co-authored-by: gvegayon <893619+gvegayon@users.noreply.github.com>
Fixed in commit a1ae2c1. The syntax error has been corrected - changed from just |
Plan for Testing Counter Name Generation
The goal is to create a test that verifies the names generated by
counter_formula()andcounter_generic()are correct, especially for models with Markov order greater than 1.Tasks Checklist
Bug Fixes Applied
Name generation fix (commit 9974afb): Fixed to show each time point separately instead of grouping all prior states together.
Syntax error fix (this commit): Fixed invalid C++ syntax where
>was used alone instead ofname += ">";. The code now properly uses>as the arrow symbol (simplified from UTF-8⇨or LaTeX arrows).Current output:
'Motif {A-}>{A+}>{A-}'✅'Motif {A+, C+}>{A+, C-}>{A-, C+}'✅All tests pass successfully.
Original prompt
We need to create a test that checks that the names generated by the
[Chronological Review: The conversation began with the user identifying an issue with the `defm_motif_parser` function, specifically its inability to handle models with more than two timestamps. The user provided examples of working and non-working cases. They suggested updating the string parser to accommodate more complex cases and requested a new test file. The user then shifted focus to creating a test for the `counter_formula()` and `counter_generic()` functions, emphasizing the need to validate the names generated for motifs with a Markov order greater than 1.]counter_formula()andcounter_generic()are as expected. I am especially concerned by the case in which we have a model with markov order greater than 1. For instance, if we have a motif that features two transitions like"{0y0} > {y0} > {0y0}", not only the positions should be OK, but also the name that is created for the motif. We can use the methodget_name()from theCounterclass to extract the name that was created. I have seen issues where using a model with markov order 2 generates a simple name with only two transitions.[Intent Mapping:
defm_motif_parserdoes not work when dealing with models with more than two timestamps." - User wants to address a limitation in the parser.counter_formula()andcounter_generic()are as expected." - User wants to ensure the naming convention for motifs is correct, especially for Markov order greater than 1.][Technical Inventory:
defm_motif_parser,counter_formula(),counter_generic(),get_name()/workspaces/barry/tests/15-defm-counts.cpp,/workspaces/barry/tests/16-defm-counts-with-formulas.cpp][Code Archaeology:
16-defm-counts-with-formulas.cpp15-defm-counts.cppcounter_formula()andcounter_generic().][Progress Assessment:
[Context Validation: All necessary context for continuing work on the parser and test creation is captured.]
[Recent Commands Analysis:
counter_formula()andcounter_generic().1. Conversation Overview: - Primary Objectives: - "The current implementation of `defm_motif_parser` does not work when dealing with models with more than two timestamps." - "I think we may need to update (generalize) how the string parser reads the data..." - "We need to create a test that checks that the names generated by the `counter_formula()` and `counter_generic()` are as expected." - Session Context: The user identified issues with the `defm_motif_parser` and requested updates and new tests to ensure proper functionality and naming conventions for motifs. - User Intent Evolution: The user initially focused on fixing the parser and then shifted to validating the naming conventions for motifs with higher Markov orders.- Technical Foundation:
- Codebase Status:
- File Name:
- Purpose: Testing DEFM motif formulas.
- Current State: Contains tests for various motif transitions.
- Key Code Segments: Examples of working and non-working motifs.
- File Name:
- Purpose: To be modified for new tests related to
- Current State: Pending modifications for new tests.
- Problem Resolution:
- Issues Encountered: The
- Solutions Implem...
defm_motif_parser: Function that needs updating to handle more complex motifs.counter_formula(),counter_generic(): Functions whose output names need validation.get_name(): Method from theCounterclass used to extract motif names.16-defm-counts-with-formulas.cpp15-defm-counts.cppcounter_formula()andcounter_generic().defm_motif_parserdoes not handle models with more than two timestamps correctly.Created from VS Code via the GitHub Pull Request extension.
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.