Add spatial-temporal metadata embeddings to CropModel#1334
Conversation
Species distributions vary by location and season. This adds an optional late-fusion pathway that encodes (lat, lon, date) as a small sinusoidal embedding (~1.5% of image features) concatenated with ResNet-50 features before the classifier. Metadata is fully optional — the model gracefully degrades to image-only predictions when metadata is absent. Key changes: - SpatialTemporalEncoder with sinusoidal encoding + small MLP - CropModel.create_model splits into backbone + classifier when use_metadata=True - MetadataImageFolder wraps ImageFolder with CSV sidecar for training - BoundingBoxDataset supports per-crop metadata for inference - predict_tile accepts metadata dict (lat, lon, date) - Config: use_metadata, metadata_dim, metadata_dropout - 19 new tests covering encoder, model, datasets, and integration - Documentation for CropModel docs and configuration reference Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
I changed something small, I don't agree with bugbot. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reapply the CropModel metadata changes on current main so the PR branch can merge cleanly.
Co-authored-by: Cursor <cursoragent@cursor.com>
|
@jveitchmichaelis can you skim this. There was a lot of cursor generated code at the beginning and I'm starting to take ownership over it as it seems to be effective and well balanced when I tested it in the BOEM repo.
There are implementation pieces I'm not sure about, and then there are scientific things that merit further review. Like what happens if we see some that is not supposed to occur in a new case, I want to manually change a few test items to see if the metadata overrides the vision model. It's a small portion of the concat features but I will test it. In terms of engineering and our workflows, I am playing with this process of working with the agents to prototype something, check it out on a different project and then assume ownership of the PR for DeepForest. That hand-off is probably awkward. I want to see which things strike you as needing developer time. @ethanwhite just keeping you abreast as a trial pattern. I'm taking it out of WIP, but not triggering review yet? |
Co-authored-by: Cursor <cursoragent@cursor.com>
| else: | ||
| meta_dim = self.classifier.in_features - image_features.shape[1] | ||
| meta_features = torch.zeros( | ||
| x.shape[0], meta_dim, device=x.device, dtype=x.dtype |
There was a problem hiding this comment.
@jveitchmichaelis let's discuss the use of x.device here, flagging this as something that worries me with #1391
There was a problem hiding this comment.
The device move here could be to the device that self.backbone is on, not to x (although if x is incorrect, we would fail on L519 anyway).
But this comes through predict_step which should handle that? So mirroring .x should be fine.
There was a problem hiding this comment.
updated to self.backbone.device
|
Okay, I have taken control over this branch. I am happy with the commits with the exception of the two comments i've flagged. I want to make sure this makes sense if we every want a transformer backbone to the crop classification model (not detection) and whether we learned anything about pytorch lightning handles the .device internally within single/multi-gpu. Other than that, testing on the BOEM pipeline shows this works nicely and I think users will appreciate this as a first step towards greater ecological realism. Getting this merged will allows us to begin to experiment with pretraining and the grain and intensity of the metadata prior. |
…ropModel docs and reduced unneeded tests Co-authored-by: Cursor <cursoragent@cursor.com>
85d8247 to
919c84d
Compare
| else: | ||
| meta_dim = self.classifier.in_features - image_features.shape[1] | ||
| meta_features = torch.zeros( | ||
| x.shape[0], meta_dim, device=x.device, dtype=x.dtype |
There was a problem hiding this comment.
The device move here could be to the device that self.backbone is on, not to x (although if x is incorrect, we would fail on L519 anyway).
But this comes through predict_step which should handle that? So mirroring .x should be fine.
| x, y = batch | ||
| outputs = self.forward(x) | ||
| if len(batch) == 3: | ||
| x, y, metadata = batch |
There was a problem hiding this comment.
In future we could probably have this more cleanly as batch.x, batch.y, batch.metadata and have the collation handle it. As opposed to a conditional here and having to set metadata anyway.
There was a problem hiding this comment.
Let's do this in a separate PR if needed, since it's not related to the behavior of the current collate.
|
Picking this back up, once it passes, @jveitchmichaelis merge if you are ready. |
4285986 to
26ff5be
Compare
There was a problem hiding this comment.
I had a re-read:
- Some comments about docs where things could be expanded upon / also docstrings
- A few comments about the architecture of the changes. The modeling looks fine, but it's getting a bit messy with conditions everywhere. I would just make a separate subclass or a wrapper as we do with the dataset. Let's look at that later.
- Needs some tests for the dataset + check that embeddings are computed in range etc?
| crop_model.trainer.validate(crop_model) | ||
|
|
||
| # Save checkpoint | ||
| crop_model.trainer.save_checkpoint("model.ckpt") |
There was a problem hiding this comment.
Do we have an example for loading? The difference between lightning checkpoint, huggingface model + config in a folder, and a plain torch checkpoint can be confusing for users.
|
|
||
| When `use_metadata=True`, the CropModel: | ||
|
|
||
| 1. Encodes `(lat, lon, day_of_year)` |
There was a problem hiding this comment.
Add a bit more detail to how the encoding works (e.g. position/rotation)
| result = m.predict_tile( | ||
| path="image.tif", | ||
| crop_model=crop_model, | ||
| metadata={"lat": 35.2, "lon": -120.4, "date": "2024-06-15"} |
There was a problem hiding this comment.
Should probably specify somewhere what requirements are for this - are all necessary, only some?
|
|
||
| A `CropModel` is a PyTorch Lightning object and can also be used like any other model. | ||
| - `filename` matches the image basename | ||
| - `date` is an ISO format string |
There was a problem hiding this comment.
What format/convetion lat/lon?
|
|
||
| ### Sampler | ||
| The map below shows a relative metadata prior for Northern Gannet | ||
| (`Morus bassanus`) on April 15, 2024. It reflects the learned metadata branch, |
There was a problem hiding this comment.
Can we remove the LLM prose here please (also unclear to me what "It reflects the learned metadata branch, not image evidence" actually means)
| class MetadataImageFolder(Dataset): | ||
| """Wrapper that adds spatial-temporal metadata to an ImageFolder dataset. | ||
|
|
||
| Expects a CSV file with columns: filename, lat, lon, date. |
There was a problem hiding this comment.
Same as other comments - need to specify the lat lon format (e.g. decimal degrees). I suppose it doesn't matter as long as the column is consistent.
| """Wrapper that adds spatial-temporal metadata to an ImageFolder dataset. | ||
|
|
||
| Expects a CSV file with columns: filename, lat, lon, date. | ||
| The date column should be an ISO format string (e.g., "2024-06-15") |
There was a problem hiding this comment.
Would be better to use fromisoformat which is more flexible: https://docs.python.org/3/library/datetime.html#datetime.date.fromisoformat
| filepath = self.image_folder.samples[idx][0] | ||
| filename = os.path.basename(filepath) | ||
|
|
||
| if filename in self._meta_lookup: |
There was a problem hiding this comment.
Should the fallback be separate for each field? e.g. if you have lat/lon but no date.
I suggest storing the metadata (per file) as a NamedTuple which is also a bit more defensive against getting the order wrong.
| ) | ||
| use_metadata = self.config["cropmodel"].get("use_metadata", False) | ||
|
|
||
| if use_metadata: |
There was a problem hiding this comment.
It's probably transparent to the user, but feels awkward to have backbone/model/classifier and then have metadata models not have model defined and vice versa. I am OK with this for now, but I would consider sub-classing the CropModel to avoid having conditional statements everywhere.
| @@ -0,0 +1,76 @@ | |||
| """Tests for spatial-temporal metadata embeddings in CropModel.""" | |||
There was a problem hiding this comment.
Probably need a few tests that loading data with metadata columns are parsed correctly + we get sensible embeddings out (generally, add tests for the modified dataset class)
|
(also needs squash) |

Summary
Key changes
SpatialTemporalEncoder: Sinusoidal positional encoding + MLP (6 → configurable dim, default 32)CropModel: Splits into backbone + metadata encoder + classifier whenuse_metadata=TrueMetadataImageFolder: Wraps ImageFolder with a CSV sidecar (filename,lat,lon,date) for trainingBoundingBoxDataset: Supports per-crop metadata dict for inferencepredict_tile: Acceptsmetadata={"lat": ..., "lon": ..., "date": "YYYY-MM-DD"}use_metadata,metadata_dim,metadata_dropoutTest plan
pytest tests/test_metadata_cropmodel.py)pytest tests/test_crop_model.py)🤖 Generated with Claude Code
Note
Medium Risk
Touches core CropModel training/inference data flow and changes batch/forward signatures to support metadata; defaults remain unchanged, but incorrect metadata wiring or shape handling could break crop-model prediction/training in edge cases.
Overview
Adds optional spatial-temporal metadata (lat/lon/date) support to
CropModel, including a newSpatialTemporalEncoderand a backbone+metadata+classifier path whencropmodel.use_metadatais enabled, while keeping image-only behavior as the default.Training can now load per-image metadata from a sidecar CSV via
CropModel.load_from_disk(..., metadata_csv=...), and inference can pass image-level metadata throughdeepforest.predict_tile(..., metadata=...)which is expanded to per-crop tensors and threaded throughpredict/BoundingBoxDataset.Updates config/schema/docs to expose
use_metadata,metadata_dim, andmetadata_dropout, and adds a dedicated test suite covering encoder behavior, training/predict steps, dataset wrappers, and checkpoint save/load.Written by Cursor Bugbot for commit c763c17. This will update automatically on new commits. Configure here.