fix: improve ebios radar for colliding points #1403
Conversation
WalkthroughThe changes introduce a new inner function Changes
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/ebios_rm/helpers.py (1)
63-65: Consider improving point distribution strategy.The current implementation might still result in overlapping points when multiple stakeholders share the same category and exposure segment. Consider these improvements:
- Apply jitter to both angle and criticality for better point distribution
- Document the angle calculation constants
- Consider adding a minimum distance check between points
Here's a suggested approach:
+ # Constants for angle calculations + QUADRANT_SIZE = 45 # Size of each category quadrant in degrees + SEGMENTS_PER_QUADRANT = 4 # Number of exposure segments per quadrant + SEGMENT_SIZE = QUADRANT_SIZE / SEGMENTS_PER_QUADRANT # Size of each segment # For current data - angle = angle_offset[sh.category] + ( - get_exposure_segment_id(c_exposure) * (45 / 4) - ) + base_angle = angle_offset[sh.category] + ( + get_exposure_segment_id(c_exposure) * SEGMENT_SIZE + ) + angle = add_jitter(base_angle, max_jitter=SEGMENT_SIZE/4)Would you like me to provide a more detailed implementation that includes minimum distance checks between points?
Also applies to: 83-85
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/ebios_rm/helpers.py(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: ruff (3.12)
- GitHub Check: test (3.12)
- GitHub Check: build (3.12)
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: startup-docker-compose-test
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
backend/ebios_rm/helpers.py (2)
3-3: LGTM!The
randommodule import is appropriate for generating jitter values.
47-47: LGTM! Variable renaming is consistent.The renaming from
angle_offsetstoangle_offsetis appropriate as it better represents a single mapping of categories to their angle offsets. The change is consistently applied across all usages.Also applies to: 63-63, 83-83
backend/ebios_rm/helpers.py
Outdated
| def add_jitter(value, max_jitter=5): | ||
| """Add a small random offset to prevent perfect overlaps""" | ||
| return value + random.uniform(-max_jitter, max_jitter) |
There was a problem hiding this comment.
💡 Codebase verification
The review's suggestion to use jitter is valid, but its premises need correction
The add_jitter function is actually defined locally within ecosystem_radar_chart_data, not as a standalone helper. While it's correctly implemented, you're right that it's not being used yet. The suggestion to apply it to prevent overlapping points is valid, particularly for the criticality values:
- vector = [c_criticality, angle, c_exposure_val, str(sh)]
+ vector = [add_jitter(c_criticality), angle, c_exposure_val, str(sh)]The suggested docstring improvements would also be beneficial.
🔗 Analysis chain
The add_jitter function is defined but never used.
While the function implementation is sound, it's currently not being utilized in the code to prevent the colliding points mentioned in the PR objectives.
Consider applying the jitter to the critical points in the radar chart. For example:
- vector = [c_criticality, angle, c_exposure_val, str(sh)]
+ vector = [add_jitter(c_criticality), angle, c_exposure_val, str(sh)]Additionally, consider enhancing the function with:
- More detailed docstring explaining the purpose and parameters
- Input validation for
max_jitter - Documentation for the chosen default value of 5
def add_jitter(value, max_jitter=5):
- """Add a small random offset to prevent perfect overlaps"""
+ """Add a small random offset to prevent overlapping points in the radar chart.
+
+ Args:
+ value (float): The original value to add jitter to
+ max_jitter (float, optional): Maximum jitter range (+/-). Defaults to 5.
+ Chosen based on typical radar chart scale.
+
+ Returns:
+ float: Original value with random jitter applied
+
+ Raises:
+ ValueError: If max_jitter is less than or equal to 0
+ """
+ if max_jitter <= 0:
+ raise ValueError("max_jitter must be positive")
return value + random.uniform(-max_jitter, max_jitter)Let's verify if there are any similar jitter implementations in the codebase:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar jitter implementations
rg -i "jitter|random.uniform"
Length of output: 1216
Script:
#!/bin/bash
# Search for direct usage of add_jitter and imports from helpers
echo "=== Searching for direct usage of add_jitter ==="
rg "add_jitter\(" -A 2
echo -e "\n=== Searching for imports from helpers that might include add_jitter ==="
rg "from.*helpers.*import" -A 2
Length of output: 2343
Script:
#!/bin/bash
# Search for specific imports of add_jitter and examine radar chart implementation
echo "=== Searching for specific imports of add_jitter ==="
rg "from.*ebios_rm.helpers.*import.*add_jitter" -A 2
echo -e "\n=== Examining radar chart data implementation ==="
rg -A 20 "def ecosystem_radar_chart_data" backend/ebios_rm/helpers.py
Length of output: 976
Script:
#!/bin/bash
# Get the complete implementation of ecosystem_radar_chart_data
ast-grep --pattern 'def ecosystem_radar_chart_data($$$) {
$$$
}'
Length of output: 68
Script:
#!/bin/bash
# Get the complete implementation with more context
echo "=== Complete ecosystem_radar_chart_data implementation ==="
rg -A 50 "def ecosystem_radar_chart_data" backend/ebios_rm/helpers.py
Length of output: 1999
Script:
#!/bin/bash
# Get the complete implementation with much more context
echo "=== Complete ecosystem_radar_chart_data implementation ==="
rg -A 100 "def ecosystem_radar_chart_data" backend/ebios_rm/helpers.py
Length of output: 3165
ab-smith
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
backend/ebios_rm/helpers.py (1)
65-65:⚠️ Potential issueApply jitter to criticality values instead of angles.
Applying jitter to angles could distort the radar chart's quadrant structure. To prevent overlapping points while maintaining the chart's integrity:
- Apply jitter to criticality values instead of angles
- Remove the large jitter value (10) which could cause significant distortion
Apply this diff:
- vector = [c_criticality, add_jitter(angle, 10), c_exposure_val, str(sh)] + vector = [add_jitter(c_criticality), angle, c_exposure_val, str(sh)] - vector = [r_criticality, add_jitter(angle, 10), r_exposure_val, str(sh)] + vector = [add_jitter(r_criticality), angle, r_exposure_val, str(sh)]Also applies to: 85-85
🧹 Nitpick comments (1)
backend/ebios_rm/helpers.py (1)
9-11: Enhance the jitter function with validation and documentation.While the implementation is sound, consider these improvements:
- Add input validation for
max_jitter- Enhance the docstring with parameter descriptions and return value
- Document the rationale for the default value
Apply this diff:
def add_jitter(value, max_jitter=5.0): - """Add a small random offset to prevent perfect overlaps""" + """Add a small random offset to prevent overlapping points in the radar chart. + + Args: + value (float): The original value to add jitter to + max_jitter (float): Maximum jitter range (+/-). Defaults to 5.0. + Small value chosen to maintain data integrity. + + Returns: + float: Original value with random jitter applied + + Raises: + ValueError: If max_jitter is less than or equal to 0 + """ + if max_jitter <= 0: + raise ValueError("max_jitter must be positive") return value + random.uniform(-max_jitter, max_jitter)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/ebios_rm/helpers.py(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: test (3.12)
- GitHub Check: enterprise-functional-tests (3.12, chromium)
🔇 Additional comments (2)
backend/ebios_rm/helpers.py (2)
3-3: LGTM!The
randomimport is correctly placed and necessary for the jitter functionality.
47-47: LGTM!The variable renaming from
angle_offsetstoangle_offsetis consistent with Python naming conventions for dictionaries containing single values.
Summary by CodeRabbit