Merged
Conversation
…ntified and managed within the `JobShopGraph` and `Node` classes.
* **Node Class:** * The `node_id` type hint in the `Node` class has been updated from `int` to a `tuple[str, int]`. This allows for more granular identification of nodes by their type (as a string) and a local integer ID unique within that type.
* **JobShopGraph Class:**
* **New Attribute:** Introduced `_nodes_map` (type: `dict[tuple[str, int], Node]`), a dictionary mapping node tuple IDs to their corresponding `Node` objects for quick lookup.
* The `_next_node_id` attribute has been refactored from a single integer to a dictionary (`dict[str, int]`). This dictionary now maps each node type (string) to its next available local ID, enabling type-specific ID generation.
* The `add_node` method has been updated to leverage this new `_next_node_id` dictionary, ensuring new nodes receive IDs conforming to the `(type, local_id)` tuple structure.
* The `removed_nodes` attribute and the `remove_node` method have been similarly refactored to use the dictionary-based tracking by node type and local ID, allowing for efficient management of removed nodes.
* Documentation throughout `JobShopGraph` has been updated to reflect these changes in node ID handling.
* **Tests:**
* All relevant existing tests within the `graph` module have been refactored to account for the new tuple-based node ID behavior, ensuring test suite compatibility.
* Specifically, the assertion for `NetworkXError` on removed nodes was adjusted: The test code was changed from `with pytest.raises(nx.NetworkXError): list(graph.graph.edges(last_removed_node_id))` (which incorrectly did not raise an error as `edges()` returns an empty list for non-existent nodes) to **attempting to remove an edge involving a non-existent node**, which correctly triggers `nx.NetworkXError`.
**Pending/Known Issues (to be addressed in subsequent commits/PRs):**
* Tests related to the `SingleJobShopGraphEnv` may not yet pass due to further integration work required.
* Specific fixes and enhancements for the `plot` and `reinforcement learning` modules (including `plot_disjunctive_graph` and `plot_resource_task_graph`) are still pending and will be addressed separately.
changed color mapping to fit new ids
ID consistency in directed disjunctive graph creation + style corrections
Removed nx digraph, only kept for testing and plots as property, built when called. New edge_index dict attribute contains edges per type, each edge being 2 nodes objects (src, tgt)
Documentation changes to reflect local ids (node_type, local_id)
Minor changes, more efficient nx graph creation
Work in progress
Author
|
New id changes for jobshopgraph |
This was
linked to
issues
Aug 13, 2025
Pabloo22
added
enhancement
Owner
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and well-thought-out refactoring of the JobShopGraph class, moving from a networkx-based implementation to a custom, more efficient adjacency list representation. The change of node identifiers to a (type, local_id) tuple is a solid design choice for heterogeneous graphs. The related changes in the reinforcement learning environment and visualization components are consistent with this new architecture.
My review focuses on improving encapsulation, fixing a minor typo, and cleaning up test code. I've also noted that several tests are currently skipped, which is understandable for a work-in-progress of this scale, but they should be re-enabled before the final merge. Overall, this is a great step forward for the library's architecture.
Comment on lines
83
to
94
| graph.add_edge( | ||
| scheduled_operation.operation.operation_id, | ||
| next_scheduled_operation.operation.operation_id, | ||
| graph._nodes_map[ | ||
| ("OPERATION", scheduled_operation.operation.operation_id) | ||
| ], | ||
| graph._nodes_map[ | ||
| ( | ||
| "OPERATION", | ||
| next_scheduled_operation.operation.operation_id, | ||
| ) | ||
| ], | ||
| type=EdgeType.DISJUNCTIVE, | ||
| ) |
Contributor
There was a problem hiding this comment.
The code directly accesses the private _nodes_map attribute of the JobShopGraph instance. It's better to use the public API to fetch nodes to improve encapsulation and maintainability. The JobShopGraph class provides a get_operation_node method for this purpose, which makes the code cleaner and more robust to future changes in the JobShopGraph implementation.
graph.add_edge(
graph.get_operation_node(scheduled_operation.operation.operation_id),
graph.get_operation_node(
next_scheduled_operation.operation.operation_id
),
type=EdgeType.DISJUNCTIVE,
)- Fixed untested disjunctive graph updater to account for mutable node ids - Skipped observation space tests due to variable shapes is observations - edge index is now generated on jobshopgraph - Now there can be multiple disjunctive and conjunctive edgetypes, to account for tuple edgettypes - Deleted removed_nodes key from observation, as the mask for removed nodes is created on the get_observation method of single jobshopgraph environment
skipped tests: - single and multijobshopgraph envs observation space and padding tests, due to observations now being mutable in size, causing errors on observation spaces and padding not being needed due to new observations structures more adapted to pytorch geometric's HeteroData object. Now observation key REMOVED NODES no longer exists, for it has no use, since removed node features are discarded during observation creation.
Replaces flat node feature keys in the observation dictionary with a nested 'node_features_dict' keyed by FeatureType values. Updates environment, observation space, and tests to use the new structure, improving clarity and extensibility for heterogeneous graph observations.
Local operation id added for available operations, job and machine nodes not handled yet
Standardizes node type names to lowercase throughout the codebase for consistency and correctness. Adds 'available_operations_with_ids' as an action mask to the observation dictionary in the reinforcement learning environment, updates related logic and tests, and adjusts action selection to use this mask, now actions being tuples of (operation_node_id, machine_node_id). Updates test and utility code to match the new conventions.
…Pabloo22#75) Adds functional `modular_instance_generator` and related functions. It also refactors the organization of the module and deprecates the `InstanceGenerator` interface. - Implemented duration matrix generation with validation. - Created machine matrix generation functions with and without recirculation. - Developed modular instance generator for creating job shop instances. - Added release date matrix generation with various strategies. - Introduced size selectors for dynamic job and machine counts. - Added comprehensive tests for duration matrix, machine matrix, modular instance generator, release date matrix, and size selectors. - Removed outdated test file for generation utilities.
instead of the deprecated `durations_matrix_array`
…rculation` Correct machine matrix shape and add validation for job and machine counts
…ntified and managed within the `JobShopGraph` and `Node` classes.
* **Node Class:** * The `node_id` type hint in the `Node` class has been updated from `int` to a `tuple[str, int]`. This allows for more granular identification of nodes by their type (as a string) and a local integer ID unique within that type.
* **JobShopGraph Class:**
* **New Attribute:** Introduced `_nodes_map` (type: `dict[tuple[str, int], Node]`), a dictionary mapping node tuple IDs to their corresponding `Node` objects for quick lookup.
* The `_next_node_id` attribute has been refactored from a single integer to a dictionary (`dict[str, int]`). This dictionary now maps each node type (string) to its next available local ID, enabling type-specific ID generation.
* The `add_node` method has been updated to leverage this new `_next_node_id` dictionary, ensuring new nodes receive IDs conforming to the `(type, local_id)` tuple structure.
* The `removed_nodes` attribute and the `remove_node` method have been similarly refactored to use the dictionary-based tracking by node type and local ID, allowing for efficient management of removed nodes.
* Documentation throughout `JobShopGraph` has been updated to reflect these changes in node ID handling.
* **Tests:**
* All relevant existing tests within the `graph` module have been refactored to account for the new tuple-based node ID behavior, ensuring test suite compatibility.
* Specifically, the assertion for `NetworkXError` on removed nodes was adjusted: The test code was changed from `with pytest.raises(nx.NetworkXError): list(graph.graph.edges(last_removed_node_id))` (which incorrectly did not raise an error as `edges()` returns an empty list for non-existent nodes) to **attempting to remove an edge involving a non-existent node**, which correctly triggers `nx.NetworkXError`.
**Pending/Known Issues (to be addressed in subsequent commits/PRs):**
* Tests related to the `SingleJobShopGraphEnv` may not yet pass due to further integration work required.
* Specific fixes and enhancements for the `plot` and `reinforcement learning` modules (including `plot_disjunctive_graph` and `plot_resource_task_graph`) are still pending and will be addressed separately.
Deleted observation wrapper and test for resource task graph observations, as it is no longer needed, changes are handled directly by single job shop graph env
…ntified and managed within the `JobShopGraph` and `Node` classes.
* **Node Class:** * The `node_id` type hint in the `Node` class has been updated from `int` to a `tuple[str, int]`. This allows for more granular identification of nodes by their type (as a string) and a local integer ID unique within that type.
* **JobShopGraph Class:**
* **New Attribute:** Introduced `_nodes_map` (type: `dict[tuple[str, int], Node]`), a dictionary mapping node tuple IDs to their corresponding `Node` objects for quick lookup.
* The `_next_node_id` attribute has been refactored from a single integer to a dictionary (`dict[str, int]`). This dictionary now maps each node type (string) to its next available local ID, enabling type-specific ID generation.
* The `add_node` method has been updated to leverage this new `_next_node_id` dictionary, ensuring new nodes receive IDs conforming to the `(type, local_id)` tuple structure.
* The `removed_nodes` attribute and the `remove_node` method have been similarly refactored to use the dictionary-based tracking by node type and local ID, allowing for efficient management of removed nodes.
* Documentation throughout `JobShopGraph` has been updated to reflect these changes in node ID handling.
* **Tests:**
* All relevant existing tests within the `graph` module have been refactored to account for the new tuple-based node ID behavior, ensuring test suite compatibility.
* Specifically, the assertion for `NetworkXError` on removed nodes was adjusted: The test code was changed from `with pytest.raises(nx.NetworkXError): list(graph.graph.edges(last_removed_node_id))` (which incorrectly did not raise an error as `edges()` returns an empty list for non-existent nodes) to **attempting to remove an edge involving a non-existent node**, which correctly triggers `nx.NetworkXError`.
**Pending/Known Issues (to be addressed in subsequent commits/PRs):**
* Tests related to the `SingleJobShopGraphEnv` may not yet pass due to further integration work required.
* Specific fixes and enhancements for the `plot` and `reinforcement learning` modules (including `plot_disjunctive_graph` and `plot_resource_task_graph`) are still pending and will be addressed separately.
Work in progress
Deleted observation wrapper and test for resource task graph observations, as it is no longer needed, changes are handled directly by single job shop graph env
Contributor
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 33 out of 42 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
job_shop_lib/reinforcement_learning/_single_job_shop_graph_env.py:307
- The docstring for the
resetmethod still mentions returning 'available_operations_with_ids' in the info dictionary, but the implementation on line 331 no longer includes this key. The docstring should be updated to reflect the actual return structure.
self,
*,
seed: int | None = None,
options: dict[str, Any] | None = None,
) -> tuple[ObservationDict, dict[str, Any]]:
job_shop_lib/reinforcement_learning/_single_job_shop_graph_env.py:335
- The docstring for the
stepmethod still mentions returning 'available_operations_with_ids' in the info dictionary, but the implementation no longer includes this key. The docstring should be updated to match the actual implementation.
self, action: tuple[int, int]
) -> tuple[ObservationDict, float, bool, bool, dict[str, Any]]:
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Matplotlib renders slightly differently across platforms (fonts, antialiasing, layout backends), so a baseline captured on Linux won’t match macOS pixel-for-pixel.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Jobshopgraph connectivity + id changes
Single jobshopgraph adapted to new graph (WIP)