-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
bug fixes #21
bug fixes #21
Conversation
Reviewer's Guide by SourceryThis PR implements several bug fixes and enhancements to the narrative field simulation system. The changes focus on improving emotional state processing, adding theme influence tracking, fixing memory interaction handling, and adjusting simulation parameters. Sequence diagram for process_interaction methodsequenceDiagram
participant Story1
participant Story2
participant InteractionEngine
InteractionEngine->>Story1: add_memory(memory)
InteractionEngine->>Story2: add_memory(memory)
loop for each shared theme
InteractionEngine->>Story1: update_theme_influence(theme, resonance * 0.1)
InteractionEngine->>Story2: update_theme_influence(theme, resonance * 0.1)
end
Updated class diagram for Story classclassDiagram
class Story {
+theme_influences: dict
+update_theme_influence(theme: str, influence: float)
}
note for Story "Added theme_influences attribute and update_theme_influence method"
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @leonvanbokhorst - I've reviewed your changes - here's some feedback:
Overall Comments:
- Please use a more descriptive PR title and add details in the description about the specific bugs fixed and improvements made
- The changes to simulation parameters (story count from 9->15 and iterations from 100->10) should be documented with rationale for the adjustments
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
@@ -261,6 +265,7 @@ def __init__( | |||
self._field = None | |||
if field: | |||
self.set_field(field) | |||
self.theme_influences = {} # Add this line to track theme influences |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (complexity): Consider consolidating theme influence tracking into a single location within the Story class
The theme influence tracking is unnecessarily split between Story.theme_influences and the perspective object, creating confusing duplication. Consolidate this into the Story class since it's the natural owner of theme-related state.
# Remove theme_influences from perspective object
# Keep only in Story class:
class Story:
def __init__(self, ...):
self.theme_influences = {} # Single source of truth
def update_theme_influence(self, theme: str, influence: float):
self.theme_influences[theme] = self.theme_influences.get(theme, 0) + influence
# Update get_journey_analytics() to use story.theme_influences directly
most_influential_themes = sorted(
story.theme_influences.items(),
key=lambda x: x[1],
reverse=True
)[:3]
This removes the duplicate state while preserving all functionality.
# Safely get unique interactions | ||
unique_interactions = { | ||
m["partner_id"] for m in story.memory_layer if "partner_id" in m | ||
} | ||
unique_interactions = set() | ||
for memory in story.memory_layer: | ||
if hasattr(memory, 'partner_id') and memory.partner_id is not None: | ||
unique_interactions.add(memory.partner_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (code-quality): Convert for loop into set comprehension (set-comprehension
)
# Safely get unique interactions | |
unique_interactions = { | |
m["partner_id"] for m in story.memory_layer if "partner_id" in m | |
} | |
unique_interactions = set() | |
for memory in story.memory_layer: | |
if hasattr(memory, 'partner_id') and memory.partner_id is not None: | |
unique_interactions.add(memory.partner_id) | |
unique_interactions = { | |
memory.partner_id | |
for memory in story.memory_layer | |
if hasattr(memory, 'partner_id') and memory.partner_id is not None | |
} |
Summary by Sourcery
Enhance emotional state processing and theme influence tracking, and fix unique interaction generation.
Bug Fixes:
Enhancements: