WIP: testing datagraph with test data #235
Conversation
📝 WalkthroughWalkthroughThis pull request introduces updates to the Changes
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
Documentation and Community
|
|
For a 'CT,SEG' query, it keeps a lot of empty unused columns: Can I remove the ones not needed? |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## development #235 +/- ##
============================================
Coverage 65.12% 65.12%
============================================
Files 55 55
Lines 3710 3710
============================================
Hits 2416 2416
Misses 1294 1294 ☔ View full report in Codecov by Sentry. |
![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 (4)
src/imgtools/modules/interlacer.py (4)
1-1: Remove unused import "time".
This import is never used in the code, so removing it improves cleanliness and eliminates any confusion.- import time🧰 Tools
🪛 Ruff (0.8.2)
1-1:
timeimported but unusedRemove unused import:
time(F401)
10-10: Add a class docstring for clarity.
Providing a short docstring at the class level helps future maintainers quickly understand whatSeriesNoderepresents and how it should be used.
59-59: Consider using a standard dict instead of defaultdict.
Without a specified default factory, this behaves the same as a normaldict. It may be clearer to usedictunless you plan to leveragedefaultdictfeatures.
90-96: Evaluate performance of copying branches on each recursive call.
Usingbranch.copy()in a depth-first search can be costly for large trees. Consider alternative approaches (e.g., adding/removing from the branch list or building the result once the leaf is reached) to improve performance and memory usage.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/imgtools/modules/interlacer.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`src/**/*.py`: Review the Python code for compliance with PE...
src/**/*.py: Review the Python code for compliance with PEP 8 and PEP 257 (docstring conventions). Ensure the following: - Variables and functions follow meaningful naming conventions. - Docstrings are present, accurate, and align with the implementation. - Code is efficient and avoids redundancy while adhering to DRY principles. - Consider suggestions to enhance readability and maintainability. - Highlight any potential performance issues, edge cases, or logical errors. - Ensure all imported libraries are used and necessary.
src/imgtools/modules/interlacer.py
🪛 Ruff (0.8.2)
src/imgtools/modules/interlacer.py
1-1: time imported but unused
Remove unused import: time
(F401)
37-37: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: Unit-Tests (windows-latest, py313)
- GitHub Check: Unit-Tests (windows-latest, py312)
- GitHub Check: Unit-Tests (windows-latest, py311)
- GitHub Check: Unit-Tests (windows-latest, py310)
- GitHub Check: Unit-Tests (macos-13, py313)
- GitHub Check: Unit-Tests (macos-13, py312)
- GitHub Check: Unit-Tests (macos-13, py311)
- GitHub Check: Unit-Tests (macos-13, py310)
- GitHub Check: Unit-Tests (macos-latest, py310)
- GitHub Check: Unit-Tests (ubuntu-latest, py313)
- GitHub Check: Unit-Tests (ubuntu-latest, py312)
- GitHub Check: Unit-Tests (ubuntu-latest, py311)
- GitHub Check: Unit-Tests (ubuntu-latest, py310)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/imgtools/modules/interlacer.py (1)
35-37:⚠️ Potential issueAvoid using mutable default arguments.
Lists as default arguments can lead to unexpected behavior. It's safer to use
Noneand initialize a new list inside the method.+ from typing import Dict, List, Set, Optional - def __init__(self, series_nodes: List[SeriesNode] = []): + def __init__(self, series_nodes: Optional[List[SeriesNode]] = None): + if series_nodes is None: + series_nodes = [] self.series_nodes = series_nodes🧰 Tools
🪛 Ruff (0.8.2)
35-35: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
🧹 Nitpick comments (8)
src/imgtools/modules/interlacer.py (8)
1-7: Consider adding a module-level docstring.This module would benefit from a descriptive docstring at the beginning explaining its purpose, functionality, and how the classes relate to each other. This would help users understand the high-level purpose before diving into class details.
8-15: Improve attribute initialization with validation and more explicit type hints.The current implementation dynamically sets attributes from row fields, which could make it difficult to track which attributes are available on each node. Consider:
- Adding explicit type annotations for commonly used attributes
- Adding validation for required fields
- Documenting the expected structure of the input row
+ from typing import Dict, List, Set, Any, Optional class SeriesNode(): - def __init__(self, row: pd.Series): + def __init__(self, row: pd.Series): + """ + Initialize a SeriesNode from a DataFrame row. + + Parameters + ---------- + row : pd.Series + A row from a DataFrame containing at minimum 'Index', 'Modality', + 'PatientID', and 'ReferencedSeriesUID' fields. + """ self.Series: str = row.Index for field in row._fields[1:]: # Skip "Index" (first field) setattr(self, field, getattr(row, field)) + + # Validate required attributes + required_fields = ['Modality', 'PatientID'] + for field in required_fields: + if not hasattr(self, field): + raise ValueError(f"Required field '{field}' missing from row") self.children: List[SeriesNode] = []
16-19: Add type hints to add_child method.The
add_childmethod is missing parameter and return type annotations, which would improve code clarity and help with static analysis.- def add_child(self, child_node): + def add_child(self, child_node: 'SeriesNode') -> None: """Add SeriesNode to children""" self.children.append(child_node)
38-41: Add return type annotation to add_node method.The method is missing a return type annotation. Adding one would improve code clarity.
- def add_node(self, node: SeriesNode): + def add_node(self, node: SeriesNode) -> None: """Add a SeriesNode to the branch.""" self.series_nodes.append(node)
79-85: Enhance error handling for edge cases in parent-child relationship building.The current implementation assumes that referenced series will always exist in the forest. Consider adding more robust error handling and logging to handle cases where the reference doesn't exist.
if modality in ["CT", "MR"] or (modality == "PT" and pd.isna(reference_series_uid)): self.root_nodes.append(node) - if pd.notna(reference_series_uid) and reference_series_uid in self.forest: - parent_node = self.forest[reference_series_uid] - parent_node.add_child(node) + if pd.notna(reference_series_uid): + if reference_series_uid in self.forest: + parent_node = self.forest[reference_series_uid] + parent_node.add_child(node) + else: + # Handle case where reference exists but is not in forest + print(f"Warning: Referenced series {reference_series_uid} not found in forest for {series_instance_uid}")
137-149: Consider simplifying the DataFrame creation logic.The nested dictionary comprehension with multiple levels and conditions is complex. Consider breaking this down into more readable parts with intermediate variables.
- data = [ - { - 'Patient_ID': branch.series_nodes[0].PatientID, # Patient ID (same for all nodes) - **{ - f'{field}_{modality}': getattr(branch.get_modality_map().get(modality), field) - for field in ['Series', 'folder'] - for modality in queried_modalities - } - } - for branch in query_result - ] + data = [] + fields_to_extract = ['Series', 'folder'] + + for branch in query_result: + # Start with patient ID + branch_data = {'Patient_ID': branch.series_nodes[0].PatientID} + + # Get modality map once per branch + modality_map = branch.get_modality_map() + + # Add fields for each modality + for modality in queried_modalities: + if modality in modality_map: + for field in fields_to_extract: + branch_data[f'{field}_{modality}'] = getattr(modality_map[modality], field, None) + else: + # Handle missing modalities gracefully + for field in fields_to_extract: + branch_data[f'{field}_{modality}'] = None + + data.append(branch_data)
151-177: Make visualization more configurable and handle paths better.The visualization method hardcodes the output filename and doesn't provide options for customization. Consider enhancing it:
- def visualize_forest(self) -> None: + def visualize_forest(self, output_path: Optional[Path] = None, height: str = '800px', + width: str = '100%', physics_enabled: bool = True) -> Path: """ Visualizes the forest of `SeriesNode` objects as an interactive network graph. - The visualization is saved as an HTML file (`forest_visualization.html`), displaying nodes + The visualization is saved as an HTML file, displaying nodes for each `SeriesNode` and edges representing parent-child relationships. + + Parameters + ---------- + output_path : Path, optional + The path where the HTML file will be saved. If None, saves to the current + working directory with filename 'forest_visualization.html'. + height : str, default '800px' + Height of the visualization. + width : str, default '100%' + Width of the visualization. + physics_enabled : bool, default True + Whether to enable physics simulation in the visualization. + + Returns + ------- + Path + The path to the saved HTML file. """ - net = Network(height='800px', width='100%', notebook=False, directed=True) + net = Network(height=height, width=width, notebook=False, directed=True) def add_node_and_edges(node, parent=None): net.add_node(node.Series, label=node.Modality, title=node.Series) # Display Series on click if parent: net.add_edge(node.Series, parent.Series) for child in node.children: add_node_and_edges(child, node) for root in self.root_nodes: add_node_and_edges(root) net.force_atlas_2based() - net.show_buttons(filter_=['physics']) + if physics_enabled: + net.show_buttons(filter_=['physics']) - save_path = Path.cwd() / 'forest_visualization.html' + if output_path is None: + save_path = Path.cwd() / 'forest_visualization.html' + else: + save_path = output_path save_path.parent.mkdir(parents=True, exist_ok=True) net.write_html(save_path.as_posix()) print(f"Visualization saved at: {save_path}") + return save_path
99-108: Enhance _query method documentation and type safety.The method uses set comparison with
<=which may not be immediately clear. Consider enhancing the documentation and adding more explicit conditions.- def _query(self, queried_modalities: Set[str]) -> List[Branch]: + def _query(self, queried_modalities: Set[str]) -> List[Branch]: """Returns Branches that contain *all* specified modalities.""" + """ + Returns Branches that contain *all* specified modalities. + + The method uses set containment to find branches where all queried + modalities are present. + + Parameters + ---------- + queried_modalities : Set[str] + Set of modality strings to query (e.g., {'CT', 'SEG'}) + + Returns + ------- + List[Branch] + List of branches containing all specified modalities + """ result = [] for branch in self.branches: present_modalities = {node.Modality for node in branch} - if queried_modalities <= present_modalities: + # Check if all queried modalities are in this branch + if queried_modalities.issubset(present_modalities): result.append(branch) return result
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/imgtools/modules/interlacer.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`src/**/*.py`: Review the Python code for compliance with PE...
src/**/*.py: Review the Python code for compliance with PEP 8 and PEP 257 (docstring conventions). Ensure the following: - Variables and functions follow meaningful naming conventions. - Docstrings are present, accurate, and align with the implementation. - Code is efficient and avoids redundancy while adhering to DRY principles. - Consider suggestions to enhance readability and maintainability. - Highlight any potential performance issues, edge cases, or logical errors. - Ensure all imported libraries are used and necessary.
src/imgtools/modules/interlacer.py
🪛 Ruff (0.8.2)
src/imgtools/modules/interlacer.py
35-35: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: Unit-Tests (windows-latest, py313)
- GitHub Check: Unit-Tests (windows-latest, py312)
- GitHub Check: Unit-Tests (windows-latest, py311)
- GitHub Check: Unit-Tests (windows-latest, py310)
- GitHub Check: Unit-Tests (macos-13, py313)
- GitHub Check: Unit-Tests (macos-13, py312)
- GitHub Check: Unit-Tests (macos-13, py311)
- GitHub Check: Unit-Tests (macos-13, py310)
- GitHub Check: Unit-Tests (macos-latest, py310)
- GitHub Check: Unit-Tests (ubuntu-latest, py313)
- GitHub Check: Unit-Tests (ubuntu-latest, py312)
- GitHub Check: Unit-Tests (ubuntu-latest, py311)
- GitHub Check: Unit-Tests (ubuntu-latest, py310)
🔇 Additional comments (1)
src/imgtools/modules/interlacer.py (1)
111-133: Great comprehensive docstring for query method!The docstring for the query method is well structured with clear parameters, returns, and a detailed explanation of supported modalities. This is a good example of how other methods in the class should be documented.
|
nice work should be good once its ruff and mypy compliant and |
Summary by CodeRabbit
New Features
Refactor
Documentation