Flipping Latent Pixels #15
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @Xmaster6y - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 5 issues found
- 🟢 Security: all looks good
- 🟡 Testing: 2 issues found
- 🟢 Complexity: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| if self.config.data is not None: | ||
| measure_data = self.config.data[name] | ||
| else: | ||
| measure_data = None |
There was a problem hiding this comment.
suggestion (code_refinement): Consider refactoring to avoid code duplication.
The conditional block for setting 'measure_data' is repeated in both hook definitions. Consider extracting this to a method to reduce duplication and improve maintainability.
| if self.config.data is not None: | |
| measure_data = self.config.data[name] | |
| else: | |
| measure_data = None | |
| def get_measure_data(self, name): | |
| if self.config.data is not None: | |
| return self.config.data[name] | |
| return None | |
| def hook(module, input, output): | |
| measure_data = self.get_measure_data(name) |
| for move in board.legal_moves | ||
| ] | ||
| legal_move_mask[idx, legal_moves] = 1 | ||
| return legal_move_mask * board_tensor | ||
| return legal_move_mask * out_tensor |
There was a problem hiding this comment.
question (code_clarification): Variable renaming should maintain clarity.
The renaming of 'board_tensor' to 'out_tensor' should be clearly justified if it's meant to reflect a broader usage or a different kind of tensor than originally implemented.
| self.boards.append(board) | ||
| self.game_ids.append(obj["gameid"]) | ||
| self.labels.append(obj["label"]) | ||
| if first_n is not None and len(self.boards) >= first_n: | ||
| break |
There was a problem hiding this comment.
suggestion (code_refinement): Addition of 'first_n' parameter for early loop exit.
The introduction of 'first_n' to limit the number of processed items is a useful feature. Ensure that this behavior is documented, especially how it interacts with the rest of the data processing pipeline.
| self.boards.append(board) | |
| self.game_ids.append(obj["gameid"]) | |
| self.labels.append(obj["label"]) | |
| if first_n is not None and len(self.boards) >= first_n: | |
| break | |
| first_n: Optional[int] = None, | |
| ): | |
| if file_name is None: | |
| super().__init__(file_name, boards, game_ids) | |
| self.boards.append(board) | |
| self.game_ids.append(obj["gameid"]) | |
| self.labels.append(obj["label"]) | |
| if first_n is not None and len(self.boards) >= first_n: | |
| break |
| if self.config.data is not None: | ||
| measure_data = self.config.data[name] | ||
| else: | ||
| measure_data = None |
There was a problem hiding this comment.
issue (testing): No tests covering the refactored conditional logic in hooks.
The conditional logic for setting 'measure_data' based on 'self.config.data' has been moved inside the hook definitions. This change in the control flow could affect when and how 'measure_data' is set during the execution of hooks. It's important to have tests that specifically verify that 'measure_data' is correctly assigned in both 'INPUT' and 'OUTPUT' hook modes under various configurations.
| raise NotImplementedError( | ||
| "Input hook not implemented for ModifyHook" | ||
| ) |
There was a problem hiding this comment.
issue (testing): Test needed for NotImplementedError in ModifyHook.
The addition of a NotImplementedError when 'HookMode.INPUT' is selected in 'ModifyHook' is a significant change. There should be a test to ensure that this exception is raised as expected when the input hook mode is used, which helps in validating that the system behaves correctly in response to unsupported configurations.
| if modify_data is None: | ||
| return output | ||
| else: | ||
| return output * modify_data |
There was a problem hiding this comment.
suggestion (code-quality): Replace if statement with if expression (assign-if-exp)
| if modify_data is None: | |
| return output | |
| else: | |
| return output * modify_data | |
| return output if modify_data is None else output * modify_data |
| if self.config.hook_mode is HookMode.INPUT: | ||
|
|
||
| def hook(module, input, output): | ||
| if self.config.data is not None: |
There was a problem hiding this comment.
issue (code-quality): Replace if statement with if expression [×2] (assign-if-exp)
| if self.config.data is not None: | ||
| modify_data = self.config.data[name] | ||
| else: | ||
| modify_data = None |
There was a problem hiding this comment.
suggestion (code-quality): Replace if statement with if expression (assign-if-exp)
| if self.config.data is not None: | |
| modify_data = self.config.data[name] | |
| else: | |
| modify_data = None | |
| modify_data = self.config.data[name] if self.config.data is not None else None |
No description provided.