-
Notifications
You must be signed in to change notification settings - Fork 4
Update pyproject toml, Code Quality and Linting Improvements #145
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from 9 commits
Commits
Show all changes
23 commits
Select commit
Hold shift + click to select a range
3f72eb0
Modernize pyproject.toml with comprehensive tooling configuration
edeno d121e93
Remove pre-commit dependency
edeno 3c9478d
Fix ruff and mypy configuration issues
edeno 1f73e92
Update Python version requirement to 3.10+ and auto-fix ruff issues
edeno ff91eb2
Add PLAN.md for systematic ruff issue fixing
edeno ced766b
Fix Priority 1: Critical ruff issues (7 fixes)
edeno f5e7888
Fix Priority 2: Auto-fix 37 code quality issues
edeno 69182fb
Fix Priority 3: Additional style improvements (3 fixes)
edeno f401777
Fix formatting
edeno ebd3f12
Address GitHub PR review feedback
edeno ba7bc49
Update review plan - zip() uses safer strict=True default
edeno 1126f42
Fix handling of empty nwb_hw_channel_order
edeno 526931b
Improve docstrings and type annotations across modules
edeno 5c3a4af
Standardize docstrings for improved clarity
edeno 883c26b
Delete REVIEW_PLAN.md
edeno f431303
Use strict mode in zip for NWB creation loop
edeno dfa4834
Update src/trodes_to_nwb/tests/test_convert_analog.py
edeno 24d457c
Update src/trodes_to_nwb/convert_position.py
edeno 3803819
Update src/trodes_to_nwb/convert_position.py
edeno 831052a
Enforce strict zip in add_dios channel mapping
edeno 49e9788
Merge branch 'update-pyproject-toml' of https://github.com/LorenFrank…
edeno a9caf6d
Delete PLAN.md
edeno 2e0e15c
Update based on code comments
edeno File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,97 @@ | ||
| # CLAUDE.md | ||
|
|
||
| This file provides guidance to Claude Code (claude.ai/code) when working with code in this repository. | ||
|
|
||
| ## Project Overview | ||
|
|
||
| This is a Python package that converts SpikeGadgets .rec files (electrophysiology data) to NWB 2.0+ format. The conversion includes ephys data, position tracking, video files, DIO events, and behavioral metadata, with validation for DANDI archive compatibility. | ||
|
|
||
| ## Development Setup Commands | ||
|
|
||
| **Environment Setup:** | ||
|
|
||
| ```bash | ||
| mamba env create -f environment.yml | ||
| mamba activate trodes_to_nwb | ||
| pip install -e . | ||
| ``` | ||
|
|
||
| **Testing:** | ||
|
|
||
| ```bash | ||
| pytest --cov=src --cov-report=xml --doctest-modules -v --pyargs trodes_to_nwb | ||
| ``` | ||
|
|
||
| **Linting:** | ||
|
|
||
| ```bash | ||
| black . | ||
| ``` | ||
|
|
||
| **Build Package:** | ||
|
|
||
| ```bash | ||
| python -m build | ||
| twine check dist/* | ||
| ``` | ||
|
|
||
| ## Architecture | ||
|
|
||
| ### Core Conversion Pipeline | ||
|
|
||
| The main conversion happens in `src/trodes_to_nwb/convert.py` with the `create_nwbs()` function which orchestrates: | ||
|
|
||
| 1. **File Discovery** (`data_scanner.py`): Scans directories for .rec files and associated data files | ||
| 2. **Metadata Loading** (`convert_yaml.py`): Loads and validates YAML metadata files | ||
| 3. **Header Processing** (`convert_rec_header.py`): Extracts device configuration from .rec file headers | ||
| 4. **Data Conversion**: Modular converters for different data types: | ||
| - `convert_ephys.py`: Raw electrophysiology data | ||
| - `convert_position.py`: Position tracking and video | ||
| - `convert_dios.py`: Digital I/O events | ||
| - `convert_analog.py`: Analog signals | ||
| - `convert_intervals.py`: Epoch and behavioral intervals | ||
| - `convert_optogenetics.py`: Optogenetic stimulation data | ||
|
|
||
| ### File Structure Requirements | ||
|
|
||
| Input files must follow naming convention: `{YYYYMMDD}_{animal}_{epoch}_{tag}.{extension}` | ||
|
|
||
| Required files per session: | ||
|
|
||
| - `.rec`: Main recording file | ||
| - `{date}_{animal}.metadata.yml`: Session metadata | ||
| - Optional: `.h264`, `.videoPositionTracking`, `.cameraHWSync`, `.stateScriptLog` | ||
|
|
||
| ### Metadata System | ||
|
|
||
| - Uses YAML metadata files validated against JSON schema (`nwb_schema.json`) | ||
| - Probe configurations stored in `device_metadata/probe_metadata/` | ||
| - Virus metadata in `device_metadata/virus_metadata/` | ||
| - See `docs/yaml_mapping.md` for complete metadata field mapping | ||
|
|
||
| ### Key Data Processing | ||
|
|
||
| - Uses Neo library (`spike_gadgets_raw_io.py`) for .rec file I/O | ||
| - Implements chunked data loading (`RecFileDataChunkIterator`) for memory efficiency | ||
| - Parallel processing support via Dask for batch conversions | ||
| - NWB validation using nwbinspector after conversion | ||
|
|
||
| ## Testing | ||
|
|
||
| - Unit tests in `src/trodes_to_nwb/tests/` | ||
| - Integration tests in `tests/integration-tests/` | ||
| - Test data downloaded from secure UCSF Box in CI | ||
| - Coverage reports uploaded to Codecov | ||
|
|
||
| ## Release Process | ||
|
|
||
| 1. Tag release commit (e.g. `v0.1.0`) | ||
| 2. Push tag to GitHub (triggers PyPI upload) | ||
| 3. Create GitHub release | ||
|
|
||
| ## Important Notes | ||
|
|
||
| - Package supports Python >=3.8 | ||
| - Requires `ffmpeg` for video conversion | ||
| - Uses hatch for build system with VCS-based versioning | ||
| - Main branch protected, requires PR reviews | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,78 @@ | ||
| # Ruff Issues Fix Plan | ||
edeno marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| This document tracks the plan to fix the remaining 56 ruff issues (excluding notebook issues). | ||
|
|
||
| ## 🔴 Priority 1: Critical Fixes (7 issues) - ✅ COMPLETED | ||
|
|
||
| ### Immediate Action Required | ||
|
|
||
| - [x] **Mutable Default Argument** (`convert_ephys.py:42`) | ||
| - Change `nwb_hw_channel_order=[]` to `nwb_hw_channel_order=None` | ||
| - Add `if nwb_hw_channel_order is None: nwb_hw_channel_order = []` inside function | ||
|
|
||
| - [x] **Missing Raise Statements** (2 issues) | ||
| - `spike_gadgets_raw_io.py:170, 1210` - Add `raise` keyword before exception instantiation | ||
|
|
||
| - [x] **Exception Chaining** (`convert_position.py:134, 602`) | ||
| - Change `raise SomeException(...)` to `raise SomeException(...) from err` | ||
|
|
||
| - [x] **Top-Level Imports** (`convert_optogenetics.py` - 4 locations) | ||
| - Move `import` statements from inside functions to module top | ||
|
|
||
| ## 🟡 Priority 2: Code Quality (25 issues) - ✅ COMPLETED | ||
|
|
||
| ### Quick Wins - Auto-fixable patterns | ||
|
|
||
| - [x] **Dictionary/List Inefficiencies** (11 issues) | ||
| - Replace `key in dict.keys()` with `key in dict` (8 instances) | ||
| - Replace `dict()` with `{}` literals (2 instances) | ||
| - Replace list comprehension with set comprehension (1 instance) | ||
|
|
||
| - [x] **Logic Simplification** (6 issues) | ||
| - Use ternary operators for simple if/else blocks | ||
| - Use `.get()` method instead of if/else for dict access | ||
| - Replace `not a == b` with `a != b` | ||
|
|
||
| - [x] **Unused Variables** (6 issues) | ||
| - Remove unused assignments in tests | ||
| - Replace unused loop variables with `_` | ||
|
|
||
| - [x] **Unnecessary Comprehensions** (6 issues) | ||
| - Convert list comprehensions to generators where appropriate | ||
|
|
||
| ## 🟠 Priority 3: Style & Performance (9 issues remaining) - PARTIALLY COMPLETED | ||
|
|
||
| ### Consider for future refactoring | ||
|
|
||
| - [ ] **Magic Numbers** (`convert_position.py` - 4 instances) | ||
| - Extract constants: `MIN_TIMESTAMPS = 2`, `DEFAULT_TIMEOUT = 2000`, `MIN_TICKS = 100` | ||
| - **Note**: These are context-specific values that may be better left as literals | ||
|
|
||
| - [ ] **Memory Optimization** (`spike_gadgets_raw_io.py` - 4 instances) | ||
| - Replace `@lru_cache` with `@cached_property` or manual caching for methods | ||
| - **Note**: These require careful analysis to avoid breaking performance | ||
|
|
||
| - [x] **Variable Naming** (2 instances) | ||
| - Rename single-letter variables to descriptive names | ||
|
|
||
| - [x] **Other Improvements** (6 issues) | ||
| - Add stacklevel to warnings | ||
| - Use contextlib.suppress() for clean exception handling | ||
| - Remove unused imports | ||
|
|
||
| ## Progress Tracking | ||
|
|
||
| **Total Issues**: 56 (excluding notebooks) | ||
|
|
||
| - **Fixed**: 47 (7 Priority 1 + 37 Priority 2 + 3 Priority 3) | ||
| - **Remaining**: 9 (4 magic numbers + 4 memory optimizations + 1 unused import) | ||
|
|
||
| **Estimated Timeline**: | ||
|
|
||
| - Phase 1 (Critical): 30 minutes | ||
| - Phase 2 (Quality): 45 minutes | ||
| - Phase 3 (Style): As needed during regular development | ||
|
|
||
| ## Commit Strategy | ||
|
|
||
| Each priority level will be committed separately with detailed commit messages explaining the fixes applied. | ||
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
Oops, something went wrong.
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.