Conversation
There was a problem hiding this comment.
Code Review
This pull request updates example scripts with new file paths and null seeds, makes scipy imports optional in the WanMatrixGame3 runner, and adds a conditional check for seeding in the pipeline. Feedback highlights that skipping seed initialization could lead to crashes in runners expecting an integer, suggesting a random seed be generated instead. Furthermore, the use of hardcoded absolute paths in examples is discouraged for portability, and the new optional scipy imports require safety checks in the functions that utilize them to avoid runtime errors.
| if self.seed is not None: | ||
| seed_all(self.seed) |
There was a problem hiding this comment.
Setting self.seed to None and skipping seed_all will result in input_info.seed being None. This will cause a crash in runners that use torch.manual_seed(input_info.seed) (such as WanMatrixGame3Runner at line 1624). It is better to initialize self.seed with a random integer if it is None to ensure downstream compatibility.
| if self.seed is not None: | |
| seed_all(self.seed) | |
| if self.seed is None: | |
| self.seed = torch.randint(0, 2**32, (1,)).item() | |
| seed_all(self.seed) |
| pipe.generate( | ||
| seed=200, | ||
| save_result_path="/data/nvme1/yongyang/FL/LightX2V/save_results/output_lightx2v_neopp_dense_2k_0.png", | ||
| save_result_path="/data/nvme1/yongyang/kkk/LightX2V/save_results/output_lightx2v_neopp_dense_2k_0.png", |
| try: | ||
| from scipy.interpolate import interp1d | ||
| from scipy.spatial.transform import Rotation, Slerp | ||
| except ImportError: | ||
| interp1d = None | ||
| Rotation = None | ||
| Slerp = None |
There was a problem hiding this comment.
Wrapping these imports in a try-except block without corresponding checks in the functions that use them (e.g., _matrix_game3_interpolate_camera_poses) will lead to a TypeError at runtime if scipy is missing. If scipy is an optional dependency, please add checks to provide a more helpful error message when these features are accessed.
No description provided.