Skip to content

Added support for temporal segmentation data in encoder decoder factory #355

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 20 commits into from
May 22, 2025

Conversation

rijuld
Copy link
Contributor

@rijuld rijuld commented Jan 9, 2025

No description provided.

@Joao-L-S-Almeida
Copy link
Member

Joao-L-S-Almeida commented Jan 9, 2025

Hi, @rijuld Maybe you would prefer to convert your PR to a draft and add the prefix [WIP] (work in progress) while work on it.
When you feel it's ready to be merged, you can convert it back to PR and add me as reviewer.

@rijuld rijuld changed the title Added support for temporal segmentation data in encoder decoder factory [DO NOT MERGE] [WIP] Added support for temporal segmentation data in encoder decoder factory Jan 9, 2025
@rijuld rijuld marked this pull request as draft January 9, 2025 16:41
@rijuld
Copy link
Contributor Author

rijuld commented Jan 9, 2025

Hi, @rijuld Maybe you would prefer to convert your PR to a draft and add the prefix [WIP] (work in progree) while works on it. When you feel it's ready to be merged, you can convert it back to PR and add me as reviewer.

@Joao-L-S-Almeida Thanks, Done.

@Joao-L-S-Almeida
Copy link
Member

Thank you for your contribution.

@blumenstiel
Copy link
Collaborator

@rijuld Thanks for working on this feature, that will be very helpful!
To pass the checks, please remember to sign off your next commits (either git commit -s or VS Code/PyCharm have some checkboxes for it to do it automatically)
See https://github.com/IBM/terratorch/pull/355/checks?check_run_id=35365530364

@romeokienzler
Copy link
Collaborator

Dear @rijuld - this has gone stale, any chance to address the conflicts and comments? Then we can merge. You can also join the TerraTorch community calls or ping us on Slack

@rijuld rijuld marked this pull request as ready for review April 10, 2025 14:11
@Joao-L-S-Almeida Joao-L-S-Almeida self-assigned this May 12, 2025
@Foxigod
Copy link
Contributor

Foxigod commented May 14, 2025

Hi, do we have an ETA for this feature/PR merge?

@rijuld rijuld changed the title [WIP] Added support for temporal segmentation data in encoder decoder factory Added support for temporal segmentation data in encoder decoder factory May 14, 2025
@Joao-L-S-Almeida
Copy link
Member

Joao-L-S-Almeida commented May 14, 2025

@Foxigod I don't know what ETA means in this context.

@rijuld I tested your implementation and it seems it's working as expected. For example:

def test_temporal_wrapper_prithvi_forward_shapes(dummy_encoder):

    NUM_CHANNELS = 6
    encoder = BACKBONE_REGISTRY.build("prithvi_eo_v2_300")

    wrapper = TemporalWrapper(encoder)
    batch_size = 2

    # Test case 2: Valid input shape
    x = torch.randn(batch_size, NUM_CHANNELS, 4, 224, 224)  # [B, C, T, H, W]
    output = wrapper(x)
    print([o.shape for o in output])

produces 24 tensors with size [2, 197, 1024]. It would be nice if you could add some tests for backbones available in the repository, as prithvi_eo_v2_300 and prithvi_swin_B.

@paolo-fraccaro Do you think the convention adopted by @rijuld for this wrapper, (B, C,T,H,W), is the most convenient way to use it or it should have a different order, as (B,T,C,H,W) ?

@Joao-L-S-Almeida Joao-L-S-Almeida added the enhancement New feature or request label May 14, 2025
@Joao-L-S-Almeida Joao-L-S-Almeida self-requested a review May 15, 2025 00:29
@romeokienzler
Copy link
Collaborator

@Joao-L-S-Almeida ETA -> Estimated Time of Arrival -> when will it be merged
@rijuld -> can we get some tests as we need to make sure the test coverage doesn't decline

In case u need this urgently we merge today in the hope we still get the tests :)

@rijuld
Copy link
Contributor Author

rijuld commented May 20, 2025

@Joao-L-S-Almeida ETA -> Estimated Time of Arrival -> when will it be merged @rijuld -> can we get some tests as we need to make sure the test coverage doesn't decline

In case u need this urgently we merge today in the hope we still get the tests :)

@romeokienzler Sure, how can I check test coverage while testing?

@Foxigod
Copy link
Contributor

Foxigod commented May 20, 2025

@romeokienzler This functionality would be beneficial for our case, and I believe I've heard @paolofraccaro mention the same thing. For my case, I don't need it today, but in the next week or two would be best if you want to finalize the tests before merging.

@naomi-simumba
Copy link
Collaborator

naomi-simumba commented May 20, 2025

@Joao-L-S-Almeida, for the current use cases we have, switching to a [B, T, C, H, W] format would be more convenient.
Also, I have tested this for 4 timestamps with the following backbones (with format switched to [B, T, C, H, W] and image size 224):

  • prithvi_eo_v2_300/prithvi_eo_v2_600
  • ssl4eos12_resnet50_sentinel2_all_decur
  • ssl4eos12_resnet50_sentinel2_all_dino

@romeokienzler, I think behaviour with Swin models needs further testing before merge. For example, satlas_swin_b_sentinel2_si_ms.

@Joao-L-S-Almeida
Copy link
Member

Hi, @rijuld
You can use a command like:
pytest -s --cov=terratorch -v --cov-report term-missing tests (you need the dependencies under test in the pyproject.toml) running inside the root terratorch directory.
In any case, it's more important to verify if the backbones are working with the new approach than the coverage itself.

Signed-off-by: João Lucas de Sousa Almeida <[email protected]>
@Joao-L-S-Almeida
Copy link
Member

Joao-L-S-Almeida commented May 22, 2025

@rijuld I added two modifications to your fork as a PR. I need you approve them in order to we finish the first part of this task. I think after it we can merge and maybe continue in a new PR after it.

@rijuld
Copy link
Contributor Author

rijuld commented May 22, 2025

@rijuld I added two modifications to your fork as a PR. I need you approve them in order to we finish the first part of this task. I think after it we can merge and maybe continue in a new PR after it.

Hi @Joao-L-S-Almeida I have approved the workflow

@Joao-L-S-Almeida
Copy link
Member

Thanks, @rijuld. The tests passed. Could you merge them ?

Adding two basic tests for Swin and modifying the encoder-decoder factory to support embeddings as tuples.
@rijuld
Copy link
Contributor Author

rijuld commented May 22, 2025

Hi @Joao-L-S-Almeida done

@Joao-L-S-Almeida
Copy link
Member

@rijuld Thank you for it. I'll merge it, since it seems alright for now. Feel free to add any others changes and contributions in new PRs.

@Joao-L-S-Almeida Joao-L-S-Almeida merged commit 33d0b3e into IBM:main May 22, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.1 enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for non-temporal encoders to run on temporal datasets in EncoderDecoderFactory
6 participants