Adding a basic example model for experimentation and testing.#3
Adding a basic example model for experimentation and testing.#3
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR replaces a basic example module with functions with a more comprehensive example PyTorch model for experimentation and testing purposes. The change shifts from simple greeting/meaning functions to a complete neural network model implementation.
- Removes the existing example module containing greeting and meaning functions along with their tests
- Adds a new ExampleModel class that implements a PyTorch neural network with hyrax model registry integration
- Includes a default configuration file for the model parameters
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/external_hyrax_example/test_example_module.py | Removes tests for the old example module functions |
| src/external_hyrax_example/example_module.py | Removes the old example module with greeting and meaning functions |
| src/external_hyrax_example/example_model.py | Adds new PyTorch model class with training logic and tensor conversion |
| src/external_hyrax_example/default_config.toml | Adds configuration file for model parameters |
| src/external_hyrax_example/init.py | Updates imports to expose ExampleModel instead of old functions |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
|
||
| def __init__(self, config, data_sample=None): | ||
| """Basic initialization with architecture definition""" | ||
| super().__init__() |
There was a problem hiding this comment.
Missing input validation for data_sample parameter. The code assumes data_sample is not None and has the expected nested structure, but could fail if data_sample is None or missing the 'data'/'image' keys.
| super().__init__() | |
| super().__init__() | |
| # Input validation for data_sample | |
| if ( | |
| data_sample is None | |
| or not isinstance(data_sample, dict) | |
| or "data" not in data_sample | |
| or not isinstance(data_sample["data"], dict) | |
| or "image" not in data_sample["data"] | |
| or not hasattr(data_sample["data"]["image"], "shape") | |
| ): | |
| raise ValueError( | |
| "data_sample must be a dict with structure: {'data': {'image': <array with .shape>}}" | |
| ) |
|
|
||
| def forward(self, x): | ||
| """Standard PyTorch forward method""" | ||
| return self.linear(x) |
There was a problem hiding this comment.
The forward method expects a flattened tensor but receives raw input. The tensor needs to be flattened before passing to the linear layer. Should be return self.linear(x.view(x.size(0), -1)).
| return self.linear(x) | |
| return self.linear(x.view(x.size(0), -1)) |
| image = data_dict["data"]["image"][0] | ||
| label = data_dict["data"]["label"] | ||
| return (torch.tensor(image, dtype=torch.float32), torch.tensor(label, dtype=torch.float32)) |
There was a problem hiding this comment.
Missing input validation and unclear indexing logic. The code assumes data_dict has the expected structure and that 'image' is indexable, but there's no validation. The [0] indexing is also unclear - consider adding a comment explaining why only the first element is taken.
| image = data_dict["data"]["image"][0] | |
| label = data_dict["data"]["label"] | |
| return (torch.tensor(image, dtype=torch.float32), torch.tensor(label, dtype=torch.float32)) | |
| # Validate input structure | |
| if not isinstance(data_dict, dict): | |
| raise ValueError("Expected data_dict to be a dict.") | |
| if "data" not in data_dict or not isinstance(data_dict["data"], dict): | |
| raise ValueError("Expected data_dict to have a 'data' key containing a dict.") | |
| data = data_dict["data"] | |
| if "image" not in data or "label" not in data: | |
| raise ValueError("Expected 'data' dict to contain 'image' and 'label' keys.") | |
| image = data["image"] | |
| # Assume 'image' is a batch; take the first sample | |
| if not hasattr(image, "__getitem__") or len(image) == 0: | |
| raise ValueError("'image' must be a non-empty indexable object (e.g., list, array).") | |
| image_sample = image[0] | |
| label = data["label"] | |
| return (torch.tensor(image_sample, dtype=torch.float32), torch.tensor(label, dtype=torch.float32)) |
| x, y = batch | ||
| y_pred = self(x) |
There was a problem hiding this comment.
Inconsistent tensor handling between train_step and forward methods. The train_step method passes x directly to forward(), but forward() expects a flattened tensor. This will cause a shape mismatch error.
No description provided.