Pytorch support - #276
Open
cpozogonzalez wants to merge 179 commits into
Open
Pytorch support#276cpozogonzalez wants to merge 179 commits into
cpozogonzalez wants to merge 179 commits into
Conversation
…ytorch legacy code
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 36 out of 38 changed files in this pull request and generated 8 comments.
Suppressed comments (4)
ctlearn/tools/train_model.py:306
setup_frameworkis declared without aselfparameter, but is called asself.setup_framework()insetup(). This will raiseTypeError: setup_framework() takes 0 positional arguments but 1 was givenat runtime.
ctlearn/tools/train_model.py:107ctapipe.core.traits.Pathexpects the keyword argumentexists, but the code usesexits. This likely raises aTypeErrorduring import/trait initialization and prevents the tool from running.
This issue also appears on line 303 of the same file.
pyproject.toml:95
- The console-script key
ctlearn-predict-LST1=includes an extra=in the name (ctlearn-predict-LST1=). This will create an incorrectly named entry point (or fail validation), so the expectedctlearn-predict-LST1command won't be installed.
ctlearn/tools/predict_LST1.py:1052 - The
__name__guard uses"main"instead of"__main__", somain()will never run when executing this file directly.
if __name__ == "main":
main()
Comment on lines
94
to
+98
| --LST1PredictionTool.channels=cleaned_relative_peak_time \\ | ||
| --LST1PredictionTool.image_mapper_type=BilinearMapper \\ | ||
| --type_model="/path/to/your/type/ctlearn_model.cpk" \\ | ||
| --energy_model="/path/to/your/energy/ctlearn_model.cpk" \\ | ||
| --cameradirection_model="/path/to/your/direction/ctlearn_model.cpk" \\ | ||
| --type_model="/path/to/your/type/ctlearn_model(.keras/.pth)"" \\ | ||
| --energy_model="/path/to/your/energy/ctlearn_model(.keras/.pth)"" \\ | ||
| --cameradirection_model="/path/to/your/direction/ctlearn_model(.keras/.pth)"" \\ |
Comment on lines
+2459
to
+2463
| @@ -2366,4 +2460,4 @@ def stereo_tool(): | |||
| mono_tool() | |||
|
|
|||
| if __name__ == "stereo_tool": | |||
| stereo_tool() | |||
| stereo_tool() No newline at end of file | |||
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
tjarkmiener
reviewed
Sep 18, 2026
Comment on lines
+941
to
+942
| if self.framework_type == FrameworkType.PYTORCH: | ||
| from scipy.special import softmax |
Member
There was a problem hiding this comment.
scipy is a core dependency and should be move to the top of the file
Comment on lines
-2462
to
+2474
| if __name__ == "mono_tool": | ||
| mono_tool() | ||
|
|
||
| if __name__ == "stereo_tool": | ||
| stereo_tool() No newline at end of file | ||
| if __name__ == "__main__": | ||
| import sys | ||
| if "--stereo" in sys.argv: | ||
| sys.argv.remove("--stereo") | ||
| stereo_tool() | ||
| else: | ||
| mono_tool() No newline at end of file |
Member
There was a problem hiding this comment.
Please do not introduce breaking changes if not necessary.
tjarkmiener
reviewed
Sep 18, 2026
Comment on lines
+63
to
+65
| if original_task == "type": | ||
| outputs[original_task] = F.softmax(out, dim=-1) | ||
| else: | ||
| outputs[original_task] = out | ||
| # Return raw logits for all tasks, including 'type', | ||
| # because CrossEntropyLoss expects logits during training. | ||
| outputs[original_task] = out |
Member
There was a problem hiding this comment.
Thanks @cpozogonzalez! Is this fixing the gammaness distribution plot?
tjarkmiener
reviewed
Sep 18, 2026
Comment on lines
+423
to
+425
| # Set default HDF5Merger options (disable image copying by default for standard DL2 output) | ||
| self.config.setdefault("HDF5Merger", {}).setdefault("dl1_images", False) | ||
| self.config.setdefault("HDF5Merger", {}).setdefault("true_images", False) |
Member
There was a problem hiding this comment.
Please do not overwrite silently the defaults. At least a log should be written here. And I would prefer to write it in the the docstring of the tool, i.e. providing minimal instructions to the users.
This branch was successfully deployed
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.
No description provided.