Skip to content

Commit e5351fd

Browse files
lmoresilmoresi
committed
Add GitHub integration for architectural reviews
- Issue template: .github/ISSUE_TEMPLATE/architectural-review.yml - PR template: .github/PULL_REQUEST_TEMPLATE/architectural-review.md - Validation workflow: .github/workflows/architectural-review-validation.yml - Integration guide: docs/developer/GITHUB-REVIEW-INTEGRATION.md - Quick start guide: docs/developer/REVIEW-WORKFLOW-QUICK-START.md Enables structured submission, tracking, and approval of formal architectural reviews through GitHub Issues, PRs, and Projects. See: docs/developer/GITHUB-REVIEW-INTEGRATION.md for full details.
1 parent 2c0bb95 commit e5351fd

File tree

5 files changed

+1329
-0
lines changed

5 files changed

+1329
-0
lines changed

‎.github/ISSUE_TEMPLATE/architectural-review.yml‎

Lines changed: 127 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,127 @@
1+
name: Architectural Review
2+
description: Submit a formal architectural review for approval
3+
title: "[REVIEW] "
4+
labels: ["architectural-review", "review:submitted"]
5+
assignees: []
6+
7+
body:
8+
- type: markdown
9+
attributes:
10+
value: |
11+
## Architectural Review Submission
12+
13+
This template is for formal architectural reviews documented in `docs/reviews/`.
14+
These reviews cover system design, implementation rationale, and architectural decisions.
15+
16+
**See**: `docs/developer/CODE-REVIEW-PROCESS.md` for the full review process.
17+
18+
- type: input
19+
id: review-doc
20+
attributes:
21+
label: Review Document Path
22+
description: Path to review document in docs/reviews/
23+
placeholder: docs/reviews/2025-11/FUNCTION-EVALUATION-SYSTEM-REVIEW.md
24+
validations:
25+
required: true
26+
27+
- type: dropdown
28+
id: priority
29+
attributes:
30+
label: Priority
31+
description: Urgency level for this review
32+
options:
33+
- HIGH
34+
- MEDIUM
35+
- LOW
36+
validations:
37+
required: true
38+
39+
- type: dropdown
40+
id: review-type
41+
attributes:
42+
label: Review Category
43+
description: Primary focus area of this review
44+
options:
45+
- System Architecture
46+
- Code Implementation
47+
- Testing Infrastructure
48+
- Documentation
49+
- Performance Optimization
50+
validations:
51+
required: true
52+
53+
- type: textarea
54+
id: summary
55+
attributes:
56+
label: Executive Summary
57+
description: 2-3 sentence overview of what this review covers
58+
placeholder: |
59+
This review documents the merger of evaluate() and global_evaluate() code paths,
60+
introducing automatic lambdification optimization that provides ~10,000x speedup
61+
for pure SymPy expressions through cached compiled functions.
62+
validations:
63+
required: true
64+
65+
- type: textarea
66+
id: scope
67+
attributes:
68+
label: Scope & Key Changes
69+
description: What files, systems, or components are affected?
70+
placeholder: |
71+
**Files Modified**:
72+
- src/underworld3/function/functions_unit_system.py
73+
- src/underworld3/function/pure_sympy_evaluator.py
74+
75+
**Systems Affected**:
76+
- Function evaluation system
77+
- Expression unwrapping
78+
- DMInterpolation caching
79+
80+
- type: textarea
81+
id: metrics
82+
attributes:
83+
label: Key Metrics
84+
description: Quantified impacts (performance, test coverage, LOC, etc.)
85+
placeholder: |
86+
- Performance: 22s → 0.003s (7,400x faster)
87+
- Test coverage: 20 comprehensive tests, all passing
88+
- New code: ~360 lines in pure_sympy_evaluator.py
89+
90+
- type: textarea
91+
id: dependencies
92+
attributes:
93+
label: Dependencies & Blockers
94+
description: Does this review depend on or block other reviews?
95+
placeholder: |
96+
**Depends on**: None
97+
**Blocks**: #XXX (Related feature implementation)
98+
**Related**: #YYY (Units system review)
99+
100+
- type: checkboxes
101+
id: checklist
102+
attributes:
103+
label: Pre-Submission Checklist
104+
description: Ensure review is ready for formal evaluation
105+
options:
106+
- label: All referenced tests are passing
107+
required: true
108+
- label: Review document follows template structure
109+
required: true
110+
- label: Sign-off table is included
111+
required: true
112+
- label: Known limitations are documented
113+
required: true
114+
- label: Testing instructions are provided
115+
required: true
116+
- label: Tracking index (REVIEW-TRACKING-INDEX.md) is updated
117+
required: true
118+
119+
- type: textarea
120+
id: additional-context
121+
attributes:
122+
label: Additional Context
123+
description: Any other information reviewers should know
124+
placeholder: |
125+
- Background discussions: Link to GitHub Discussions
126+
- Related PRs: #XXX, #YYY
127+
- External references: Papers, benchmarks, etc.

‎.github/PULL_REQUEST_TEMPLATE/architectural-review.md‎

Lines changed: 131 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,131 @@
1+
<!--
2+
This PR template is for architectural review submissions.
3+
For regular code changes, delete this template and use the default.
4+
-->
5+
6+
## Architectural Review Submission
7+
8+
**Review Type**: [Code Implementation / System Architecture / Testing / Documentation / Performance]
9+
**Priority**: [HIGH / MEDIUM / LOW]
10+
**Review Period**: [YYYY-MM]
11+
12+
---
13+
14+
### 📄 Review Document
15+
- **File**: `docs/reviews/YYYY-MM/[NAME]-REVIEW.md`
16+
- **Related Issue**: Closes #XXX
17+
- **Tracking Index**: `docs/reviews/YYYY-MM/REVIEW-TRACKING-INDEX.md`
18+
19+
### 📋 Executive Summary
20+
<!-- 2-3 sentence overview of what this review documents -->
21+
22+
23+
24+
### 🎯 Scope
25+
**Systems Affected**:
26+
- [ ] Core solvers (Stokes, Poisson, Advection-Diffusion)
27+
- [ ] Variable system (Mesh/Swarm variables, arrays)
28+
- [ ] Units & non-dimensionalization
29+
- [ ] Parallel operations (MPI, collective operations)
30+
- [ ] Testing infrastructure
31+
- [ ] Documentation & examples
32+
- [ ] Other: _________________
33+
34+
**Files Changed**:
35+
- New files: [count]
36+
- Modified files: [count]
37+
- Total LOC: ~[count]
38+
39+
### ✅ Testing Status
40+
- [ ] All tests passing: `pixi run underworld-test`
41+
- [ ] New tests added: [count] tests
42+
- [ ] Test coverage: [X]% (or N/A)
43+
- [ ] Regression tests validated
44+
- [ ] Performance benchmarks run (if applicable)
45+
46+
**Test Results**:
47+
```bash
48+
# Command used
49+
pixi run -e default pytest tests/test_XXXX*.py -v
50+
51+
# Results summary
52+
[X] passed, [Y] failed, [Z] skipped
53+
```
54+
55+
### 📊 Key Metrics & Impact
56+
<!-- Quantify the changes: performance, coverage, complexity -->
57+
58+
**Performance**:
59+
- Before: [metric]
60+
- After: [metric]
61+
- Improvement: [X]x faster / [Y]% reduction
62+
63+
**Quality**:
64+
- Test coverage: [X]%
65+
- Code complexity: [increase/decrease/unchanged]
66+
- Documentation: [X pages / X examples]
67+
68+
### 🔍 Review Checklist (for Reviewers)
69+
70+
**Design & Architecture**:
71+
- [ ] Design rationale is clear and well-justified
72+
- [ ] Trade-offs are documented with alternatives considered
73+
- [ ] System architecture is comprehensible
74+
- [ ] Integration points are identified
75+
76+
**Implementation**:
77+
- [ ] Implementation matches documented design
78+
- [ ] Code quality meets project standards
79+
- [ ] Breaking changes are identified and justified
80+
- [ ] Backward compatibility is addressed
81+
82+
**Testing & Validation**:
83+
- [ ] Testing strategy is adequate
84+
- [ ] Test coverage is sufficient
85+
- [ ] Edge cases are covered
86+
- [ ] Performance impact is assessed
87+
88+
**Documentation**:
89+
- [ ] Known limitations are clearly documented
90+
- [ ] Benefits are quantified
91+
- [ ] User-facing changes are documented
92+
- [ ] Migration guide provided (if needed)
93+
94+
### ⚠️ Known Limitations
95+
<!-- List any current constraints or future work needed -->
96+
97+
1.
98+
2.
99+
100+
### 🔗 Dependencies & Related Work
101+
**Depends on**:
102+
- [ ] Review #XXX (must be approved first)
103+
104+
**Blocks**:
105+
- [ ] Review #YYY (waiting on this review)
106+
107+
**Related**:
108+
- Discussion: [Link to GitHub Discussion]
109+
- Prior art: [Links to related reviews]
110+
111+
---
112+
113+
## 🖊️ Sign-Off
114+
115+
**This PR merge represents formal approval of the architectural review.**
116+
117+
Reviewers: Please review the full document at `docs/reviews/YYYY-MM/[NAME]-REVIEW.md` and approve this PR only when satisfied with the design, implementation, and documentation.
118+
119+
| Role | GitHub Handle | Sign-Off Date | Status |
120+
|------|---------------|---------------|--------|
121+
| **Author** | @username | YYYY-MM-DD | ✅ Submitted |
122+
| **Primary Reviewer** | @reviewer1 | | ⏳ Pending |
123+
| **Secondary Reviewer** | @reviewer2 | | ⏳ Pending |
124+
| **Project Lead** | @lead | | ⏳ Pending |
125+
126+
---
127+
128+
### 📝 Additional Context
129+
<!-- Optional: Any other information reviewers should know -->
130+
131+

0 commit comments

Comments
 (0)