Fix contributing guide outdated references (fixes #52) #54
Conversation
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
…es with unit tests
Add new image compressors: JPEG2000, JPEG XL, and WebP - Implemented JPEG2000Compressor with configurable quality and lossless options. - Implemented JPEGXLCompressor with support for lossless compression and adjustable effort. - Implemented WebPCompressor with options for quality, lossless mode, and compression method. - Updated __init__.py to include new compressors in the module. - Added unit tests for JPEG2000, JPEG XL, and WebP compressors to ensure functionality. - Updated test script to include tests for new compressors. - Updated requirements to include necessary dependencies for new compressors.
- Introduced SampleDataLoader class to unify loading of dataset and standard test images. - Removed the old load_sample_images function and integrated its functionality into the new class. - Updated example scripts and tests to utilize the new SampleDataLoader interface. - Added methods for downloading and caching standard test images. - Enhanced error handling and added support for resizing images during loading. - Removed the deprecated download_example_images.py script.
- Added ModelConfig class for unified model configuration management. - Updated BaseModel to support configuration objects (PretrainedConfig, DictConfig, dict). - Introduced KairaTrainer for unified training of communication models with flexible argument handling. - Implemented TrainingArguments class to support Hydra and dict-based configurations. - Added tests for new dataset loading functionalities and improved error handling in image downloading. - Updated requirements to include necessary libraries for new features.
…asses; enhance documentation and error handling in sample data tests
- Introduced test_init.py to validate imports and exports in kaira.data module. - Enhanced test_sample_data.py to include tests for download_image function. - Created test_training.py with extensive coverage for Trainer and TrainingArguments classes, including various initialization scenarios and configuration handling. - Added tests for extracting configuration values and ensuring proper parameter filtering in TrainingArgumentsMixin. - Verified integration between different components of the training module.
…e training configurations for models
…nce dataset filtering
- Updated `kaira_train.py` to streamline model loading and configuration parsing. - Removed unnecessary parameters from `load_model_from_config` function. - Simplified configuration loading logic using Hydra and OmegaConf. - Enhanced `create_training_arguments_from_args` to support direct CLI argument parsing. - Removed extensive test cases for training arguments and trainer due to redundancy. - Ensured all tests are aligned with the new configuration handling approach.
- Updated kaira_train.py to streamline model loading and configuration parsing - Removed unnecessary parameters from load_model_from_config function - Simplified configuration loading logic using Hydra and OmegaConf - Enhanced create_training_arguments_from_args to support direct CLI argument parsing - Removed extensive test cases for training arguments and trainer due to redundancy - Ensured all tests are aligned with the new configuration handling approach (Cherry-picked from 8f2293c with conflict resolution)
…ization across examples
…dels in API reference
…decoder listings and improving encoder descriptions
… to redundancy and refactoring.
…lity - Renamed dataset classes to simplify naming conventions (e.g., BinaryTensorDataset to BinaryDataset). - Updated test cases to reflect new class names and structures. - Enhanced tests for BinaryDataset, UniformDataset, GaussianDataset, and CorrelatedDataset to ensure proper functionality and reproducibility. - Removed deprecated classes and tests related to old dataset structures. - Consolidated test cases for plotting functions to reflect new performance plotting methods. - Removed unused sample data tests and streamlined the testing process for image datasets. - Improved error handling and validation in dataset loading functions.
- Rearranged import statements in various files for better organization. - Simplified dataset initialization and sample generation in `plot_data_generation.py`. - Updated plotting functions to use consistent formatting and style. - Enhanced comments and docstrings for clarity across multiple modules. - Consolidated performance plotting calls in FEC simulation scripts. - Improved test cases for dataset classes to ensure robustness. - Cleaned up unnecessary whitespace and formatting inconsistencies in several files.
- Deleted test files for ECC benchmarks, benchmark runners, and targeted coverage. - This cleanup removes legacy tests that are no longer relevant to the current benchmarking framework.
- Replace non-existent 'bash scripts/lint.sh' with correct pre-commit commands - Create missing scripts/lint.sh that calls pre-commit hooks - Update installation instructions to remove non-existent [dev] extra - Add pre-commit setup instruction to development environment setup - Fix reference to non-existent automated_example_gallery.md file - Ensure lint.sh is executable and works with both direct execution and Makefile Fixes #52
Remove reference to non-existent docs/automated_example_gallery.md file and replace with more accurate guidance to check existing examples.
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR addresses issue #52 by updating the contributing guide to reflect the current development workflow and removing outdated references throughout the codebase.
Key Changes
- Removed entire benchmark subsystem including comprehensive test suites, CLI tools, and documentation references
- Created missing
scripts/lint.shscript that properly calls pre-commit hooks, making it executable and compatible with Makefile usage - Replaced benchmark CLI with training CLI by adding
kaira_train.pyand updating console script entry point
Reviewed Changes
Copilot reviewed 92 out of 113 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/benchmarks/ | Removed all benchmark test files as part of subsystem cleanup |
| setup.py | Updated console script entry point from kaira-benchmark to kaira-train |
| scripts/kaira_train.py | Added comprehensive training CLI with Hydra config support and Hub integration |
| scripts/lint.sh | Created missing linting script that calls pre-commit hooks |
| kaira/training/ | Added new training module with TrainingArguments and Trainer classes |
| kaira/models/image/compressors/ | Added comprehensive image compression classes (JPEG, PNG, WebP, etc.) |
| scripts/generate_*.py | Updated to remove benchmark references from documentation generation |
Comments suppressed due to low confidence (1)
Comment on lines
+483
to
+484
| # Setup Hugging Face Hub upload if requested | ||
| hub_config = setup_hub_upload(args) |
There was a problem hiding this comment.
This Hub setup call appears to be duplicated from line 414. The hub_config is already set up earlier in the function and doesn't need to be called again.
Suggested change
| # Setup Hugging Face Hub upload if requested | |
| hub_config = setup_hub_upload(args) |
Comment on lines
+295
to
+298
| if len(returns) == 1: | ||
| return returns[0] | ||
| else: | ||
| return tuple(returns) |
There was a problem hiding this comment.
[nitpick] Consider using a more explicit return pattern. The tuple unpacking logic could be simplified by consistently returning tuples and letting callers handle unpacking.
Suggested change
| if len(returns) == 1: | |
| return returns[0] | |
| else: | |
| return tuple(returns) | |
| return tuple(returns) |
Comment on lines
+1319
to
+1325
| axes[0].imshow(original.permute(1, 2, 0).numpy()) | ||
| axes[0].set_title("Original", fontweight="bold") | ||
| axes[0].axis("off") | ||
|
|
||
| # Results at different conditions | ||
| for i, (condition, result) in enumerate(results_dict.items()): | ||
| axes[i + 1].imshow(result.permute(1, 2, 0).numpy().clip(0, 1)) |
There was a problem hiding this comment.
Consider adding error handling for tensor to numpy conversion. The tensor might be on GPU or have unsupported data types.
Suggested change
| axes[0].imshow(original.permute(1, 2, 0).numpy()) | |
| axes[0].set_title("Original", fontweight="bold") | |
| axes[0].axis("off") | |
| # Results at different conditions | |
| for i, (condition, result) in enumerate(results_dict.items()): | |
| axes[i + 1].imshow(result.permute(1, 2, 0).numpy().clip(0, 1)) | |
| try: | |
| original_np = original.cpu().permute(1, 2, 0).numpy() | |
| axes[0].imshow(original_np) | |
| except Exception as e: | |
| raise ValueError(f"Failed to convert the original tensor to a NumPy array: {e}") | |
| axes[0].set_title("Original", fontweight="bold") | |
| axes[0].axis("off") | |
| # Results at different conditions | |
| for i, (condition, result) in enumerate(results_dict.items()): | |
| try: | |
| result_np = result.cpu().permute(1, 2, 0).numpy().clip(0, 1) | |
| axes[i + 1].imshow(result_np) | |
| except Exception as e: | |
| raise ValueError(f"Failed to convert the result tensor for condition '{condition}' to a NumPy array: {e}") |
Comment on lines
+152
to
+156
| if hasattr(model_class, "__init__"): | ||
| import inspect | ||
|
|
||
| sig = inspect.signature(model_class.__init__) | ||
| print(f"Model constructor signature: {sig}", file=sys.stderr) |
There was a problem hiding this comment.
All classes in Python have an init method, so this check is unnecessary. Consider removing this condition or checking for a more specific attribute.
Suggested change
| if hasattr(model_class, "__init__"): | |
| import inspect | |
| sig = inspect.signature(model_class.__init__) | |
| print(f"Model constructor signature: {sig}", file=sys.stderr) | |
| import inspect | |
| sig = inspect.signature(model_class.__init__) | |
| print(f"Model constructor signature: {sig}", file=sys.stderr) |
Codecov ReportAttention: Patch coverage is
📢 Thoughts on this report? Let us know! |
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.
This PR addresses issue #52 by updating the contributing guide to reflect the current development workflow.
Changes Made
Fixed Outdated References
bash scripts/lint.shwith correct pre-commit commands in the development workflow sectionscripts/lint.shscript that properly calls pre-commit hooks, making it executable and compatible with both direct execution and Makefile usage[dev]extra install optiondocs/automated_example_gallery.mdfileUpdated Development Workflow
pre-commit run -aandmake formatTechnical Details
scripts/lint.shthat callspre-commit run --all-fileswith proper error handlinglinttargetTesting
scripts/lint.shworks correctly when called directlymake lintworks correctly using the new scriptIssues Resolved
Fixes #52 - Contributing guide is out of date
The contributing guide now accurately reflects the current development workflow and tooling used in the project. All outdated references have been corrected and the guide provides accurate instructions for new contributors.