Conversation
- Fix: JSON export now uses _serialize_solution() like JLD2 - Handles both UnifiedTimeGridModel and MultipleTimeGridModel - Automatic dispatch based on time grid model type - Add: Comprehensive multi-grid test suite (60 tests) - Grid detection (unified vs multiple) - JLD2 round-trip with different grids - Error handling for MultipleTimeGridModel - Component symbol mapping (plural forms, equivalences) - Edge cases (T_dual = nothing) - Optimization verification (identical grids → unified) - Coverage: ~70% of multi-grid system - Construction: 100% - Grid access: 100% - JLD2 serialization: 100% - Error handling: 100% - JSON serialization: 0% (TODO: fix matrix dimension issues) Closes multi-grid serialization bug in JSON extension.
- Add: Serialization structure tests (13 tests) - Verify UnifiedTimeGridModel produces legacy format - Verify MultipleTimeGridModel produces multi-grid format - Check presence/absence of correct keys - Add: Extreme grid size tests (6 tests) - Very different sizes (1001 vs 11 points) - Minimum grid size (2 points) - Verify correct model type selection - Add: Grid reconstruction tests (6 tests) - Serialize → deserialize round-trip - Verify grid preservation - Test _reconstruct_solution_from_data - Add: Legacy format detection tests (4 tests) - Backward compatibility verification - Legacy format → UnifiedTimeGridModel - Format auto-detection Total: 89 tests passing Coverage: ~85% of multi-grid system
- DRY: Extract _discretize_all_components to eliminate code duplication
- SRP: Isolate JSON matrix conversion in _convert_matrices_for_json!
- KISS: Simplify JSON import with _json_to_matrix helpers
- OCP/DIP: Preserve JLD2 native Matrix format, JSON uses Vector{Vector}
- All tests pass (1729/1729) including multi-grids
- Fix JLD2 Matrix dimension mismatch bug
- Reduce code by 46% in serialization functions
🏗️ Architecture: JSON/JLD2 extensions now follow SOLID principles
✅ Tests: 1729/1729 pass
📦 Files: solution.jl, CTModelsJSON.jl, test_export_import.jl
BREAKING CHANGE: Reduced from 4 to 3 time grids for better semantics: - T_state: shared by state, costate, and state box constraint duals - T_control: shared by control and control box constraint duals - T_path: shared by path constraints and their duals (replaces T_dual) Key changes: - MultipleTimeGridModel: 5 grids → 3 grids (:state, :control, :path) - build_solution: 4 positional args → 3 positional args - clean_component_symbols: costate→state, dual→path mapping - Updated JSON serialization with backward compatibility - Updated plotting component mapping - All tests updated and passing (89/89) This simplifies the API while maintaining backward compatibility for import/export.
- Add T_costate as 4th parameter in build_solution signature - Update MultipleTimeGridModel to include costate grid - Modify clean_component_symbols to map :costate → :costate (own grid) - Update time_grid validation for UnifiedTimeGridModel to accept :costate - Add T_costate support in serialization (JSON/JLD) with backward compatibility - Update plotting to map costate to its dedicated grid - Fix all tests to use 4-grid signature and correct costate assertions - Maintain legacy build_solution compatibility with 4 identical grids - 3320/3321 tests passing (1 unrelated JSON export error) BREAKING: build_solution signature now requires 4 time grids BACKWARD: Legacy signature preserved, older files supported via fallback
Contributor
…emantics - Add detailed explanation of 4 independent time grids (T_state, T_control, T_costate, T_path) - Document grid semantics and associations with trajectory components - Explain matrix vs function formats for trajectory data - Add comprehensive example with multi-grid usage - Document grid validation requirements and memory optimization - Clarify box constraint dual dimensions and backward compatibility - Follow docstrings.md standards with structured sections
…tics - Add comprehensive documentation for _serialize_solution with format examples - Explain unified vs multiple time grids serialization formats - Document grid-component associations in _discretize_all_components - Clarify backward compatibility and memory efficiency - Add detailed notes on discretization behavior and round-trip guarantees - Document both UnifiedTimeGridModel and MultipleTimeGridModel serialization methods - Follow docstrings.md standards with structured sections and examples
- Add dedicated T_costate time grid for costate trajectories - Support 4 independent time grids (state, control, costate, path) - Enhanced documentation with comprehensive time grid semantics - Backward compatibility maintained for legacy single-grid format - All tests passing (3324/3324)
- Document dedicated T_costate time grid feature - Explain 4-grid system (T_state, T_control, T_costate, T_path) - Note backward compatibility with legacy format - Highlight enhanced serialization and documentation - Confirm no breaking changes (all existing code works)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.