added UniverSat wrapper support #860
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a complete UniverSat integration with model loading, raster and multimodal encoding, embedding utilities, training support, public exports, tests, registry metadata, and documentation examples. ChangesUniverSat integration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant rasterio
participant UniverSatProcessor
participant UniverSatModel
rasterio->>UniverSatProcessor: read_geotiff raster and metadata
UniverSatProcessor->>UniverSatProcessor: preprocess_image and format_batch
UniverSatProcessor->>UniverSatModel: encode formatted modalities
UniverSatModel-->>UniverSatProcessor: output tokens
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@geoai/universat.py`:
- Around line 19-20: Extract the duplicated ~/.cache/geoai/UniverSat value into
one module-level constant and update both the module-scope path setup and
universat_train to reuse it, keeping the existing path behavior unchanged.
- Around line 193-215: Update get_pca_rgb and _proj to fit a single PCA(3) model
over all batch tokens, then reuse that fitted model when transforming each tile
so RGB component orientations remain consistent across the batch. Preserve the
existing handling for single 2D inputs, square 3D inputs, normalization, and
output shapes.
- Around line 79-96: Update UniverSatProcessor.__init__ to accept the pretrained
and size options used by universat_inference, then forward them to
load_universat_model when no model instance is supplied. Preserve the existing
device placement and eval_mode behavior for explicitly provided models, while
allowing those options to select non-default pretrained model variants.
- Around line 111-135: Update _process_sample so tensor inputs follow the same
scale normalization and time-series dimension expansion as the str/array paths,
reusing preprocess_image or its equivalent processing. Revise d_val handling so
supplied {mod}_dates for custom modalities are not silently discarded: honor
them when present, or explicitly reject unsupported date metadata; preserve
default dates for recognized time-series modalities.
- Around line 18-37: Refactor the module-level UniverSat setup into an explicit
lazy helper that clones and imports only when requested, rather than during
import of geoai.universat. Pin the Git checkout to a known commit or tag, guard
cloning with a lock, and apply a finite timeout to the clone operation. Keep the
path setup and imports within the helper, and have callers obtain UniverSat and
related registry symbols through that lazy initialization.
In `@tests/test_universat.py`:
- Around line 56-70: The modality_registry mock is applied after geoai.universat
binds INPUT_RES, SUBPATCHES, and WAVELENGTHS, so the test still uses real
values. Update the test setup to install the sys.modules patch before importing
geoai.universat and constructing UniverSatProcessor, or patch the imported
constants directly on geoai.universat before use.
In `@zensical.toml`:
- Line 105: Add the new universat API documentation page to the API Reference
navigation list in zensical.toml, placing the universat module entry alongside
the existing train and utils module entries and pointing it to universat.md.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 68dc2f21-e716-439c-a91c-0a7e32cf5381
📒 Files selected for processing (7)
docs/examples/universat.ipynbdocs/universat.mdgeoai/__init__.pygeoai/foundation_models.pygeoai/universat.pytests/test_universat.pyzensical.toml
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
♻️ Duplicate comments (1)
geoai/universat.py (1)
19-27: 🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy liftPin the Git checkout, add a lock, and make cloning lazy.
As per past review comments, while the timeout and constant were added, the Git clone still executes at module import time, lacks a lock to prevent concurrent import races, and checks out an unpinned
mainbranch. This remains a security and reliability risk. Please move this logic behind an explicit lazy helper function, pin to a specific commit or tag, and use a locking mechanism (e.g.,filelock) to prevent cache corruption during concurrent clones.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@geoai/universat.py` around lines 19 - 27, The module-level UniverSat clone logic should not run during import. Move it into an explicit lazy helper, such as a dedicated cache-initialization function, that acquires a file lock before checking or populating UNIVERSAT_CACHE_DIR, clones a pinned commit or tag instead of the unpinned default branch, and preserves the existing timeout and sys.path setup after initialization; update callers to invoke the helper when UniverSat is needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@geoai/universat.py`:
- Around line 19-27: The module-level UniverSat clone logic should not run
during import. Move it into an explicit lazy helper, such as a dedicated
cache-initialization function, that acquires a file lock before checking or
populating UNIVERSAT_CACHE_DIR, clones a pinned commit or tag instead of the
unpinned default branch, and preserves the existing timeout and sys.path setup
after initialization; update callers to invoke the helper when UniverSat is
needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 76cab67c-90a3-4b70-9ae5-8bceeff9c5bb
📒 Files selected for processing (1)
geoai/universat.py
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
geoai/universat.py (1)
146-167: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winValidate date metadata consistently across the batch.
For custom modalities, this condition can produce
Nonefor samples without{mod}_datesand tensors for samples that provide them. Becauseformat_batchchecks onlyprocessed[0][1], dates supplied by later samples are silently dropped when the first sample lacks dates, ortorch.stackfails when the first sample has dates but a later one does not. Require dates for every sample once any sample supplies them, or synthesize a documented default consistently before stacking.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@geoai/universat.py` around lines 146 - 167, Update format_batch and its date-processing flow around _process_sample so date metadata is consistent for every sample in a modality before stacking. If any sample supplies mod_dates, require all samples to provide compatible dates (or apply the documented default uniformly); otherwise omit the date tensor. Do not rely only on processed[0][1], and preserve the existing modality batching behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@geoai/universat.py`:
- Around line 23-30: Update the git clone invocation in the module’s cache setup
to resolve the executable explicitly and fail clearly when git is unavailable,
rather than relying on PATH lookup. Update format_batch() to inspect all
processed records for *_dates metadata, preserving later values and ensuring
mixed batches are handled without stacking errors.
---
Outside diff comments:
In `@geoai/universat.py`:
- Around line 146-167: Update format_batch and its date-processing flow around
_process_sample so date metadata is consistent for every sample in a modality
before stacking. If any sample supplies mod_dates, require all samples to
provide compatible dates (or apply the documented default uniformly); otherwise
omit the date tensor. Do not rely only on processed[0][1], and preserve the
existing modality batching behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 740c9302-c9e1-4c80-8595-bda0061b7c03
📒 Files selected for processing (1)
geoai/universat.py
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
geoai/universat.py (1)
200-204: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winPrevent
TypeErrorcrash when stacking mixed date metadata.If a batch contains samples where only some include
_datesfor a specific modality,processedwill contain a mix ofTensorandNonevalues forp[1]. Passing this mixed list totorch.stackwill raise aTypeErrorand crash the application.Backfill the
Nonevalues with default zero tensors (matching the length of the tensorp[0], exactly as_process_sampledoes) to ensure consistent batching.🐛 Proposed fix for stacking logic
- if any(p[1] is not None for p in processed): - batch[f"{mod}_dates"] = torch.stack([p[1] for p in processed]).to( - self.device - ) + if any(p[1] is not None for p in processed): + batch[f"{mod}_dates"] = torch.stack([ + p[1] if p[1] is not None else torch.zeros(p[0].shape[0], dtype=torch.long) + for p in processed + ]).to(self.device)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@geoai/universat.py` around lines 200 - 204, Update the date batching logic before torch.stack in the processed-result flow to replace each None date with a zero tensor whose length matches the corresponding p[0] tensor, consistent with _process_sample. Preserve existing date tensors and stack the normalized values into the modality’s _dates batch field.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@geoai/universat.py`:
- Around line 24-28: Add a module-level threading.Lock and use it in _setup to
guard the _repo_ready check and all repository initialization work, ensuring
only one thread can clone or checkout the cache at a time while preserving the
early return for already-initialized state.
---
Outside diff comments:
In `@geoai/universat.py`:
- Around line 200-204: Update the date batching logic before torch.stack in the
processed-result flow to replace each None date with a zero tensor whose length
matches the corresponding p[0] tensor, consistent with _process_sample. Preserve
existing date tensors and stack the normalized values into the modality’s _dates
batch field.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ec5e9309-7dd9-4b9e-9a31-7a9dbf6b731e
📒 Files selected for processing (2)
geoai/universat.pytests/test_universat.py
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
geoai/universat.py (1)
29-78: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winFix lock scope to prevent race condition during initialization.
The
with _setup_lock:context manager only wraps the double-checkif _repo_ready: returnblock and releases the lock immediately afterward. The rest of the initialization logic runs entirely outside the lock, which allows multiple threads to race and concurrently execute thegit cloneand path setup.Indent the remaining setup logic so it executes while the lock is held.
🔒️ Proposed fix to expand the lock scope
with _setup_lock: if _repo_ready: return - git = shutil.which("git") - if not git: - raise FileNotFoundError("git not found on PATH") - - if not os.path.exists(UNIVERSAT_CACHE_DIR): - subprocess.run( - [ - git, - "clone", - "--single-branch", - "https://github.com/gastruc/UniverSat.git", - UNIVERSAT_CACHE_DIR, - ], - check=True, - timeout=300, - ) - # pin to known working commit - subprocess.run( - [ - git, - "-C", - UNIVERSAT_CACHE_DIR, - "checkout", - "f6df2eec54955b0f7524cc95fe21a5e80c0239d9", - ], - check=True, - timeout=60, - ) - - sys.path = [UNIVERSAT_CACHE_DIR, _src] + [ - p for p in sys.path if p not in (UNIVERSAT_CACHE_DIR, _src) - ] - - try: - import torch._dynamo - - torch._dynamo.config.disable = True - except ImportError: - pass - - global UniverSat, hubconf, WAVELENGTHS - from hubconf import UniverSat # noqa: F811 - import hubconf # noqa: F811 - from modality_registry import WAVELENGTHS # noqa: F811 - - _repo_ready = True + git = shutil.which("git") + if not git: + raise FileNotFoundError("git not found on PATH") + + if not os.path.exists(UNIVERSAT_CACHE_DIR): + subprocess.run( + [ + git, + "clone", + "--single-branch", + "https://github.com/gastruc/UniverSat.git", + UNIVERSAT_CACHE_DIR, + ], + check=True, + timeout=300, + ) + # pin to known working commit + subprocess.run( + [ + git, + "-C", + UNIVERSAT_CACHE_DIR, + "checkout", + "f6df2eec54955b0f7524cc95fe21a5e80c0239d9", + ], + check=True, + timeout=60, + ) + + sys.path = [UNIVERSAT_CACHE_DIR, _src] + [ + p for p in sys.path if p not in (UNIVERSAT_CACHE_DIR, _src) + ] + + try: + import torch._dynamo + + torch._dynamo.config.disable = True + except ImportError: + pass + + global UniverSat, hubconf, WAVELENGTHS + from hubconf import UniverSat # noqa: F811 + import hubconf # noqa: F811 + from modality_registry import WAVELENGTHS # noqa: F811 + + _repo_ready = True🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@geoai/universat.py` around lines 29 - 78, Expand the _setup_lock context in the initialization function to cover all setup operations, including git discovery, repository cloning and checkout, sys.path updates, torch configuration, imports, and setting _repo_ready. Keep the existing early return under the same lock so only one thread can perform initialization while others observe the completed state.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@geoai/universat.py`:
- Around line 29-78: Expand the _setup_lock context in the initialization
function to cover all setup operations, including git discovery, repository
cloning and checkout, sys.path updates, torch configuration, imports, and
setting _repo_ready. Keep the existing early return under the same lock so only
one thread can perform initialization while others observe the completed state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 459423e1-ed25-46f0-bca0-ea4f6568c6bd
📒 Files selected for processing (1)
geoai/universat.py
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| def _setup(): | ||
| global _repo_ready | ||
| if _repo_ready: | ||
| return | ||
| with _setup_lock: | ||
| if _repo_ready: | ||
| return | ||
|
|
||
| git = shutil.which("git") | ||
| if not git: | ||
| raise FileNotFoundError("git not found on PATH") | ||
|
|
||
| if not os.path.exists(UNIVERSAT_CACHE_DIR): | ||
| subprocess.run( | ||
| [ | ||
| git, | ||
| "clone", | ||
| "--single-branch", | ||
| "https://github.com/gastruc/UniverSat.git", | ||
| UNIVERSAT_CACHE_DIR, | ||
| ], | ||
| check=True, | ||
| timeout=300, | ||
| ) | ||
| # pin to known working commit | ||
| subprocess.run( | ||
| [ | ||
| git, | ||
| "-C", | ||
| UNIVERSAT_CACHE_DIR, | ||
| "checkout", | ||
| "f6df2eec54955b0f7524cc95fe21a5e80c0239d9", | ||
| ], | ||
| check=True, | ||
| timeout=60, | ||
| ) | ||
|
|
||
| sys.path = [UNIVERSAT_CACHE_DIR, _src] + [ | ||
| p for p in sys.path if p not in (UNIVERSAT_CACHE_DIR, _src) | ||
| ] | ||
|
|
||
| try: | ||
| import torch._dynamo | ||
|
|
||
| torch._dynamo.config.disable = True | ||
| except ImportError: | ||
| pass | ||
|
|
||
| global UniverSat, hubconf, WAVELENGTHS | ||
| from hubconf import UniverSat # noqa: F811 | ||
| import hubconf # noqa: F811 | ||
| from modality_registry import WAVELENGTHS # noqa: F811 | ||
|
|
||
| _repo_ready = True |
There was a problem hiding this comment.
Bug (high confidence): the double-checked-locking pattern doesn't actually protect the setup work.
The with _setup_lock: block only wraps the inner if _repo_ready: return check (lines 29-31); the with statement ends right after that, so the git clone, sys.path mutation, dynamic imports, and the final _repo_ready = True assignment (lines 33-78) all run outside the lock. If two threads call _setup() concurrently before the repo is cached, both pass the unlocked check at line 27, both acquire/release the lock in turn without doing any work under it, and both then race to run git clone into the same UNIVERSAT_CACHE_DIR — one clone will fail (destination path already exists and is not an empty directory) or the directory can end up in an inconsistent state. This defeats the purpose of _setup_lock (added in the "threading lock fix" commit).
Fix by moving the setup body inside the with block so the lock actually serializes it:
| def _setup(): | |
| global _repo_ready | |
| if _repo_ready: | |
| return | |
| with _setup_lock: | |
| if _repo_ready: | |
| return | |
| git = shutil.which("git") | |
| if not git: | |
| raise FileNotFoundError("git not found on PATH") | |
| if not os.path.exists(UNIVERSAT_CACHE_DIR): | |
| subprocess.run( | |
| [ | |
| git, | |
| "clone", | |
| "--single-branch", | |
| "https://github.com/gastruc/UniverSat.git", | |
| UNIVERSAT_CACHE_DIR, | |
| ], | |
| check=True, | |
| timeout=300, | |
| ) | |
| # pin to known working commit | |
| subprocess.run( | |
| [ | |
| git, | |
| "-C", | |
| UNIVERSAT_CACHE_DIR, | |
| "checkout", | |
| "f6df2eec54955b0f7524cc95fe21a5e80c0239d9", | |
| ], | |
| check=True, | |
| timeout=60, | |
| ) | |
| sys.path = [UNIVERSAT_CACHE_DIR, _src] + [ | |
| p for p in sys.path if p not in (UNIVERSAT_CACHE_DIR, _src) | |
| ] | |
| try: | |
| import torch._dynamo | |
| torch._dynamo.config.disable = True | |
| except ImportError: | |
| pass | |
| global UniverSat, hubconf, WAVELENGTHS | |
| from hubconf import UniverSat # noqa: F811 | |
| import hubconf # noqa: F811 | |
| from modality_registry import WAVELENGTHS # noqa: F811 | |
| _repo_ready = True | |
| def _setup(): | |
| global _repo_ready | |
| if _repo_ready: | |
| return | |
| with _setup_lock: | |
| if _repo_ready: | |
| return | |
| git = shutil.which("git") | |
| if not git: | |
| raise FileNotFoundError("git not found on PATH") | |
| if not os.path.exists(UNIVERSAT_CACHE_DIR): | |
| subprocess.run( | |
| [ | |
| git, | |
| "clone", | |
| "--single-branch", | |
| "https://github.com/gastruc/UniverSat.git", | |
| UNIVERSAT_CACHE_DIR, | |
| ], | |
| check=True, | |
| timeout=300, | |
| ) | |
| # pin to known working commit | |
| subprocess.run( | |
| [ | |
| git, | |
| "-C", | |
| UNIVERSAT_CACHE_DIR, | |
| "checkout", | |
| "f6df2eec54955b0f7524cc95fe21a5e80c0239d9", | |
| ], | |
| check=True, | |
| timeout=60, | |
| ) | |
| sys.path = [UNIVERSAT_CACHE_DIR, _src] + [ | |
| p for p in sys.path if p not in (UNIVERSAT_CACHE_DIR, _src) | |
| ] | |
| try: | |
| import torch._dynamo | |
| torch._dynamo.config.disable = True | |
| except ImportError: | |
| pass | |
| global UniverSat, hubconf, WAVELENGTHS | |
| from hubconf import UniverSat # noqa: F811 | |
| import hubconf # noqa: F811 | |
| from modality_registry import WAVELENGTHS # noqa: F811 | |
| _repo_ready = True |
| if not os.path.exists(UNIVERSAT_CACHE_DIR): | ||
| subprocess.run( | ||
| [ | ||
| git, | ||
| "clone", | ||
| "--single-branch", | ||
| "https://github.com/gastruc/UniverSat.git", | ||
| UNIVERSAT_CACHE_DIR, | ||
| ], | ||
| check=True, | ||
| timeout=300, | ||
| ) | ||
| # pin to known working commit | ||
| subprocess.run( | ||
| [ | ||
| git, | ||
| "-C", | ||
| UNIVERSAT_CACHE_DIR, | ||
| "checkout", | ||
| "f6df2eec54955b0f7524cc95fe21a5e80c0239d9", | ||
| ], | ||
| check=True, | ||
| timeout=60, | ||
| ) |
There was a problem hiding this comment.
Bug (medium confidence): a failed/interrupted clone permanently wedges the cache.
If the first subprocess.run (clone) fails after git has already created UNIVERSAT_CACHE_DIR (e.g. network interruption, timeout), check=True raises and the exception propagates — but the directory now exists. On every subsequent call, os.path.exists(UNIVERSAT_CACHE_DIR) is True, so cloning/checkout is skipped, sys.path is still updated, and from hubconf import UniverSat fails with ImportError (or worse, imports a stale/partial tree) every time thereafter — with no automatic recovery short of the user manually deleting ~/.cache/geoai/UniverSat. Consider cloning into a temp dir and atomically renaming into place only on success, or removing the partial directory on failure.
| if t.ndim == 2: | ||
| g = int(t.shape[0] ** 0.5) | ||
| return _norm(PCA(3).fit_transform(t.reshape(-1, t.shape[-1])), g) | ||
| if t.ndim == 3 and t.shape[0] == t.shape[1]: | ||
| return _norm(PCA(3).fit_transform(t.reshape(-1, t.shape[-1])), t.shape[0]) |
There was a problem hiding this comment.
Bug (medium confidence): shape-based dispatch is ambiguous and silently mis-handles a plausible batch shape.
get_pca_rgb distinguishes "single square-grid tile" from "batch of tiles" purely by comparing t.shape[0] == t.shape[1]. But a genuine batch of tiles where the batch size equals the token count per tile is a realistic case — e.g. batching 9 tiles each encoded with output_grid=9 gives tokens.shape == (9, 81, D)... more simply, 9 tiles with output_grid=3 gives (9, 9, D). That tensor satisfies t.ndim == 3 and t.shape[0] == t.shape[1] (9 == 9), so it takes the "single tile" branch at line 265-266 and reshapes the whole (9, 9, D) batch into one (9, 9, 3) "grid", silently producing a nonsensical PCA/RGB result instead of raising an error or going through the per-tile batch path below. Since this fails silently (no exception, just wrong output), it's easy to miss. Consider requiring callers to disambiguate explicitly (e.g. always treat ndim == 3 as batch, and require callers to pass a single 2D (N, D) tensor for one tile — which matches how get_pca_rgb(tokens[0]) is used in the example notebook).
| import os, sys, subprocess, shutil, threading | ||
| from typing import Any, Dict, List, Optional, Tuple, Union | ||
| import numpy as np, torch, torch.nn as nn, rasterio |
There was a problem hiding this comment.
Quality nits (low confidence):
Union(line 4) is imported but never used in this module.- Comma-separated multi-imports on a single line (
import os, sys, subprocess, shutil, threading/import numpy as np, torch, torch.nn as nn, rasterio) is PEP 8 (E401) discouraged and inconsistent with the rest of the codebase, which uses one import per line (seegeoai/prithvi.py,geoai/dinov3.py).
| import os, sys, subprocess, shutil, threading | |
| from typing import Any, Dict, List, Optional, Tuple, Union | |
| import numpy as np, torch, torch.nn as nn, rasterio | |
| import os | |
| import shutil | |
| import subprocess | |
| import sys | |
| import threading | |
| from typing import Any, Dict, List, Optional, Tuple | |
| import numpy as np | |
| import rasterio | |
| import torch | |
| import torch.nn as nn |
| def preprocess_image( | ||
| self, img: np.ndarray, mod: str, scale: Optional[float] = None | ||
| ) -> torch.Tensor: | ||
| scale = scale or 1.0 |
There was a problem hiding this comment.
Minor (low confidence): scale = scale or 1.0 treats an explicitly-passed scale=0.0 the same as scale=None (falls back to 1.0) instead of surfacing the caller's mistake (e.g. a ZeroDivisionError/ValueError). Same pattern at line 172 in _process_sample. Probably fine in practice since real scale factors are never 0, but scale if scale is not None else 1.0 would be more defensive/explicit.
Code reviewReviewed Bugs
Security
Performance
Quality
CLAUDE.md
|
#828
Summary by CodeRabbit