-
Notifications
You must be signed in to change notification settings - Fork 27
Update simulator #61
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
Update simulator #61
Conversation
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.
Pull request overview
This pull request enhances the TSNKit project by adding support for a 2D mesh network topology, refactoring topology generation code for better maintainability, and improving command-line argument handling and simulation output capabilities.
Key Changes:
- Added
mesh_2dtopology with validation for square mesh dimensions and border-based end-station placement - Refactored topology generation functions to use a shared
_convert_2darray_to_csvhelper function, eliminating code duplication - Enhanced simulation to track and export detailed flow events to CSV via a new
--outputflag - Updated command-line parsing to use optional flags (
--output,--workers,--name) with improved help descriptions
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
| tsnkit/test/debug/_common.py | Updated subprocess calls to use new optional flag format for algorithm invocation; removed unused import |
| tsnkit/simulation/tas.py | Added flow event logging with CSV export capability; fixed comment typos; updated output handling |
| tsnkit/data/generator.py | Added automatic output directory creation; updated for mesh_2d topology support; clarified help text |
| tsnkit/data/dataset_spec.py | Implemented new mesh_2d topology function; refactored topology generators to use shared helper; fixed typo in comment |
| tsnkit/core/_config.py | Fixed typo in comment (casue → cause) |
| tsnkit/core/_common.py | Converted output/workers/name to optional flags; added automatic output directory creation |
| doc/source/dataprep.md | Added units to parameter descriptions; documented mesh_2d topology option |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if not file_name: | ||
| file_name = "log.csv" | ||
| pd.DataFrame(output, | ||
| columns=["stream", "link", "di", "time"] |
Copilot
AI
Nov 28, 2025
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.
Unclear column name and missing documentation: The output DataFrame uses a column named "di" (line 374) without any documentation explaining its meaning. Based on the code, "di" appears to be a direction indicator (0 for arrival/receive, 1 for transmission/send), but this should be documented either as a comment near the DataFrame creation or in the argument parser help text for --output.
tsnkit/data/dataset_spec.py
Outdated
|
|
||
| result = _convert_2darray_to_csv(net, num_node, num_queue, data_rate) | ||
| result.to_csv(header + ".csv", index=False) | ||
| print(result) |
Copilot
AI
Nov 28, 2025
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.
Debug print statement should be removed: This print(result) statement appears to be leftover debug code that should not be in production.
| print(result) |
|
|
||
| parser.add_argument("task", type=str, help="The file path to the stream CSV file.") | ||
| parser.add_argument("net", type=str, help="The file path to the network CSV file.") | ||
| # Switch to optional flags for all parameters |
Copilot
AI
Nov 28, 2025
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.
Misleading comment: The comment "Switch to optional flags for all parameters" is inaccurate since task and net remain as positional arguments, not optional flags. Either update the comment to reflect that only output, workers, and name are optional, or make all arguments truly optional with --task and --net flags.
| # Switch to optional flags for all parameters | |
| # Switch to optional flags for output, workers, and name; task and net remain positional |
tsnkit/data/dataset_spec.py
Outdated
| num_node = num_sw * 2 ## TODO: |ES| != |SW| for mesh_2d | ||
| net = np.zeros(shape=(num_node, num_node)) | ||
|
|
||
| if int(np.sqrt(num_sw)) ** 2 != num_sw: | ||
| raise ValueError("Wrong num_sw for mesh_2d, col_len != row_len") | ||
|
|
||
| row_len = col_len = int(np.sqrt(num_sw)) |
Copilot
AI
Nov 28, 2025
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.
Incorrect number of nodes for mesh_2d topology: The code sets num_node = num_sw * 2, assuming equal numbers of switches and end-stations. However, for a 2D mesh topology, end-stations are only placed on border nodes. For a square mesh of side length n (where num_sw = n²), the number of border positions is 4n - 4 (or equivalently (n-1)*4 as used in generator.py line 66), not n². This means the actual number of nodes should be num_sw + 4*sqrt(num_sw) - 4, not num_sw * 2. This mismatch will cause the net array to be incorrectly sized and potentially lead to out-of-bounds array access or wasted memory.
| num_node = num_sw * 2 ## TODO: |ES| != |SW| for mesh_2d | |
| net = np.zeros(shape=(num_node, num_node)) | |
| if int(np.sqrt(num_sw)) ** 2 != num_sw: | |
| raise ValueError("Wrong num_sw for mesh_2d, col_len != row_len") | |
| row_len = col_len = int(np.sqrt(num_sw)) | |
| n = int(np.sqrt(num_sw)) | |
| num_node = num_sw + 4 * n - 4 # Correct number of nodes for mesh_2d topology | |
| net = np.zeros(shape=(num_node, num_node)) | |
| if n ** 2 != num_sw: | |
| raise ValueError("Wrong num_sw for mesh_2d, col_len != row_len") | |
| row_len = col_len = n |
|
|
||
| def match_time(t: int, gcl: List[Tuple[int, int, int]]) -> int: | ||
| """Match the entry in GCl with the given time. Implemented by binary search. | ||
| """Match the entry in GCl with the given time. Implemented in binary search. |
Copilot
AI
Nov 28, 2025
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.
Typo in comment: "Implemented in binary search" should be "Implemented using binary search" or "Implemented with binary search" to be grammatically correct.
| """Match the entry in GCl with the given time. Implemented in binary search. | |
| """Match the entry in GCl with the given time. Implemented using binary search. |
tsnkit/simulation/tas.py
Outdated
| @@ -1,11 +1,13 @@ | |||
| import argparse | |||
| from email import header | |||
Copilot
AI
Nov 28, 2025
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.
Unused import: from email import header should be removed as header is never used in this file.
| from email import header |
tsnkit/simulation/tas.py
Outdated
| file_name = "log.csv" | ||
| pd.DataFrame(output, | ||
| columns=["stream", "link", "di", "time"] | ||
| ).to_csv("/".join([path_name, file_name]), index=False) No newline at end of file |
Copilot
AI
Nov 28, 2025
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.
Bug in file path construction: Using "/".join([path_name, file_name]) will fail when path_name is an empty string (when output is just a filename without path). This would produce an incorrect path starting with /. Use os.path.join(path_name, file_name) or handle the empty path_name case explicitly.
| ).to_csv("/".join([path_name, file_name]), index=False) | |
| ).to_csv(os.path.join(path_name, file_name), index=False) |
tsnkit/data/dataset_spec.py
Outdated
| return | ||
| searched.add((x,y)) | ||
|
|
||
| # Add es on boarder |
Copilot
AI
Nov 28, 2025
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.
Typo in comment: "boarder" should be "border".
| # Add es on boarder | |
| # Add es on border |
tsnkit/core/_common.py
Outdated
| if parsed.output: | ||
| os.makedirs(parsed.output, exist_ok=True) |
Copilot
AI
Nov 28, 2025
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.
[nitpick] Redundant condition check: The condition if parsed.output: is unnecessary since --output has a default value of "./", which will always be truthy. This check will always evaluate to True. If the intent is to only create the directory when explicitly provided by the user, remove the default value or track whether the argument was explicitly set.
| if parsed.output: | |
| os.makedirs(parsed.output, exist_ok=True) | |
| os.makedirs(parsed.output, exist_ok=True) |
| parser.add_argument( | ||
| "workers", type=int, nargs="?", help="The number of workers.", default=1 | ||
| "--output", | ||
| type=str, | ||
| default="./", | ||
| nargs="?", | ||
| help="The output folder path.", | ||
| ) | ||
| parser.add_argument( | ||
| "name", type=str, nargs="?", help="The name of the experiment.", default="-" | ||
| "--workers", | ||
| type=int, | ||
| default=1, | ||
| nargs="?", | ||
| help="The number of workers.", | ||
| ) | ||
| parser.add_argument( | ||
| "--name", | ||
| type=str, | ||
| default="-", | ||
| nargs="?", | ||
| help="The name of the experiment.", | ||
| ) |
Copilot
AI
Nov 28, 2025
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.
[nitpick] Unnecessary nargs="?" for optional arguments with defaults: The nargs="?" is redundant for optional arguments (--output, --workers, --name) that already have default values specified. The nargs="?" is typically used when you want to distinguish between "flag not provided", "flag provided without value", and "flag provided with value". Since these are optional arguments with defaults, remove nargs="?" for cleaner argument parsing.
This pull request introduces several improvements and new features to the TSNKit project, focusing on enhanced topology support, code refactoring for maintainability, and improved command-line usability. The most significant changes include the addition of a new 2D mesh topology, refactoring of network topology generation code, updates to command-line argument parsing, and enhancements to simulation output handling.
Topology Features and Refactoring
mesh_2d) in the network dataset specification, including validation for square mesh dimensions and border end-station placement. The list of available topologies (TOPO_FUNC) and related documentation have been updated accordingly. [1] [2] [3] [4] [5]line,ring,tree,mesh, and newmesh_2d) to use a shared helper function_convert_2darray_to_csv, reducing code duplication and improving maintainability. [1] [2] [3] [4] [5]Command-Line and Usability Improvements
tsnkit/core/_common.pyto use optional flags for all parameters, improved help descriptions, and ensured output directory creation. [1] [2] [3]Simulation Output Enhancements
--outputflag. Output directories are created automatically if needed. [1] [2] [3] [4] [5] [6] [7] [8]Minor Fixes and Cleanups
These changes collectively improve the flexibility, usability, and maintainability of the TSNKit toolkit, especially for generating and simulating complex network topologies.