Clay v1 base #136

myscon · 2024-08-27T20:06:15Z

No description provided.

Signed-off-by: myscon <[email protected]>

Joao-L-S-Almeida · 2024-09-05T14:33:12Z

The tests are failing due to an import for vit_pytorch and I understand that it should came from here. Could it be replaced by some builtin module or should we add it to our requirements ?

Signed-off-by: myscon <[email protected]>

myscon · 2024-09-05T18:42:13Z

Okay I removed vit-torch. I seem to have omitted the sign off and that was the reason for the force.

Signed-off-by: myscon <[email protected]>

Joao-L-S-Almeida · 2024-09-06T14:27:52Z

Some of the automatic tests are still failing. Maybe it could be interesting to run pytest -s tests locally and check if there any missing stuff.

Signed-off-by: myscon <[email protected]>

myscon · 2024-09-06T15:00:22Z

Made some small adjustments to the test file. They pass locally.

Signed-off-by: João Lucas de Sousa Almeida <[email protected]>

Joao-L-S-Almeida · 2024-09-17T20:03:24Z

@myconv I saw that the problem was related to the device. I modified it to "cpu" (the GitHub runner was not configured with GPU) and the tests are passing. Please, check the PR #160, try to update your fork and we can proceed with the review.

CarlosGomes98 · 2024-09-18T20:51:37Z

terratorch/models/backbones/clay_v1/embedder.py

+                 img_size=256,
+                 num_frames=1,
+                 ckpt_path=None,
+                 device="cuda",


Device shouldn't be required to be passed. In pytorch lightning, .to(device) statements should not be used, as the responsibility to place modules in the correct device is passed to lightning.

CarlosGomes98 · 2024-09-18T20:51:55Z

terratorch/models/backbones/clay_v1/embedder.py

+                heads=12,
+                dim_head=64,
+                mlp_ratio=4.0,
+            ).to(device)


Same as above, such statements should be removed

Makes sense. Those have been removed.

Signed-off-by: João Lucas de Sousa Almeida <[email protected]>

CarlosGomes98

Added some comments. This is quite a big task, so we may want to think about how we can split work somehow :)

CarlosGomes98 · 2024-09-19T16:45:56Z

terratorch/models/backbones/clay_v1/embedder.py

+                 img_size=256,
+                 num_frames=1,
+                 ckpt_path=None,
+                 bands=["blue", "green", "red", "nir", "swir16", "swir22"],


Mutable data structures should not be used for argument defaults. Consider None and in the method body explicitly check for None and replace it with the desired default. https://docs.astral.sh/ruff/rules/mutable-argument-default/

Also, if compatible, consider using the HLSBands Enum instead of strings.

CarlosGomes98 · 2024-09-19T16:46:53Z

terratorch/models/backbones/clay_v1/embedder.py

+
+        # for use in features list. Single layer feature for simplicity
+        self.feature_info.append(
+            {"num_chs": 768, "reduction": 1, "module": f"clay_encoder"})


no need for f-string in module

CarlosGomes98 · 2024-09-19T16:48:14Z

terratorch/models/clay_model_factory.py

@@ -190,12 +160,14 @@ def build_model(
                task, backbone, decoder, head_kwargs, prepare_features_for_image_model, rescale=rescale
            )

-        to_be_aux_decoders: list[AuxiliaryHeadWithDecoderWithoutInstantiatedHead] = []
+        to_be_aux_decoders: list[AuxiliaryHeadWithDecoderWithoutInstantiatedHead] = [
+        ]


closing bracket can be on the same line

CarlosGomes98 · 2024-09-19T16:49:07Z

terratorch/models/clay_model_factory.py

@@ -57,7 +59,7 @@ def build_model(
        backbone: str | nn.Module,
        decoder: str | nn.Module,
        in_channels: int,
-        bands: list[HLSBands | int],
+        bands: list[int] = [],


Mutable data structure should not be used here. See other comment on a simlar issue for details

Also, consider allowing the HLSBands Enum to be used

CarlosGomes98 · 2024-09-19T16:50:46Z

terratorch/models/clay_model_factory.py

-                # When the model is not on HG, it needs be restored locally.
-                print("This model is not available on HuggingFace. Trying to instantiate locally ...")
+            except Exception as e:
+                print(e, "Error loading from HF. Trying to instantiate locally ...")

                assert checkpoint_path, "A checkpoint must be provided to restore the model."


Prefer checking and throwing an exception if not. Assertions can be removed when python is run with optimizations

CarlosGomes98 · 2024-09-19T16:59:14Z

tests/test_clay_tasks.py

+
+
+@pytest.mark.parametrize("backbone", ["clay_v1_base"])
+@pytest.mark.parametrize("decoder", ["FCNDecoder", "UperNetDecoder", "IdentityDecoder"])


CarlosGomes98 · 2024-09-19T16:59:25Z

tests/test_clay_tasks.py

+
+
+@pytest.mark.parametrize("backbone", ["clay_v1_base"])
+@pytest.mark.parametrize("decoder", ["FCNDecoder", "UperNetDecoder", "IdentityDecoder"])


FCN and upernet probably dont belong here

CarlosGomes98 · 2024-09-19T17:03:16Z

terratorch/models/backbones/clay_v1/modules.py

+os.environ["TORCH_CUDNN_V8_API_DISABLED"] = "1"
+
+# central wavelengths of pretrained model
+WAVELENGTHS = {


could this use HLSBands for the keys?

CarlosGomes98 · 2024-09-19T17:04:12Z

terratorch/models/backbones/clay_v1/modules.py

+        qkv = self.to_qkv(x).chunk(3, dim=-1)
+        q, k, v = map(lambda t: rearrange(
+            t, 'b n (h d) -> b h n d', h=self.heads), qkv)
+


Suggested change

(rearrange(

t, 'b n (h d) -> b h n d', h=self.heads) for t in qkv)

may be simpler

CarlosGomes98 · 2024-09-19T17:05:33Z

terratorch/models/backbones/clay_v1/modules.py

+        patches, waves_encoded = self.to_patch_embed(
+            cube, waves
+        )  # [B L D] - patchify & create embeddings per patch
+        # TODO: Add time & latlon as encoding to patches


is this still todo?

myscon added 4 commits August 26, 2024 11:08

added clay_v1_base_model

07d5f98

Signed-off-by: myscon <[email protected]>

Merge branch 'main' into clay-v1-base

e0f3e79

added pytest suite; removed band requirement for clay

a97b2ad

Signed-off-by: myscon <[email protected]>

fill in blocks for datacube

1a018c7

Signed-off-by: myscon <[email protected]>

myscon force-pushed the clay-v1-base branch from b664398 to 1a018c7 Compare August 27, 2024 20:31

Merge branch 'IBM:main' into clay-v1-base

38ba4da

Joao-L-S-Almeida approved these changes Sep 5, 2024

View reviewed changes

remove vit-torch; added waves to datacuber

b064c02

Signed-off-by: myscon <[email protected]>

myscon force-pushed the clay-v1-base branch from d7f9363 to b064c02 Compare September 5, 2024 18:41

include bands in factory

94cc0c4

Signed-off-by: myscon <[email protected]>

test revisions to include bands in factory

8c1644c

Signed-off-by: myscon <[email protected]>

Joao-L-S-Almeida added 4 commits September 17, 2024 15:59

Merge branch 'main' into clay-v1-base

2ba841f

Merging with main

a7b86dd

Signed-off-by: João Lucas de Sousa Almeida <[email protected]>

Merging with main

3b0b61e

Signed-off-by: João Lucas de Sousa Almeida <[email protected]>

Trying to use CPU

a4e199d

Signed-off-by: João Lucas de Sousa Almeida <[email protected]>

CarlosGomes98 reviewed Sep 18, 2024

View reviewed changes

the device should be chosen by Lightning

0f3d3dd

Signed-off-by: João Lucas de Sousa Almeida <[email protected]>

myscon force-pushed the clay-v1-base branch from b96b580 to 0f3d3dd Compare September 18, 2024 23:38

CarlosGomes98 reviewed Sep 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clay v1 base #136

Clay v1 base #136

myscon commented Aug 27, 2024

Joao-L-S-Almeida commented Sep 5, 2024

myscon commented Sep 5, 2024

Joao-L-S-Almeida commented Sep 6, 2024

myscon commented Sep 6, 2024

Joao-L-S-Almeida commented Sep 17, 2024

CarlosGomes98 Sep 18, 2024

CarlosGomes98 Sep 18, 2024

myscon Sep 18, 2024

CarlosGomes98 left a comment

CarlosGomes98 Sep 19, 2024

CarlosGomes98 Sep 19, 2024

CarlosGomes98 Sep 19, 2024

CarlosGomes98 Sep 19, 2024

CarlosGomes98 Sep 19, 2024

CarlosGomes98 Sep 19, 2024

CarlosGomes98 Sep 19, 2024

CarlosGomes98 Sep 19, 2024

CarlosGomes98 Sep 19, 2024

CarlosGomes98 Sep 19, 2024

CarlosGomes98 Sep 19, 2024



		@pytest.mark.parametrize("backbone", ["clay_v1_base"])
		@pytest.mark.parametrize("decoder", ["FCNDecoder", "UperNetDecoder", "IdentityDecoder"])


	(rearrange(
	t, 'b n (h d) -> b h n d', h=self.heads) for t in qkv)

Clay v1 base #136

Are you sure you want to change the base?

Clay v1 base #136

Conversation

myscon commented Aug 27, 2024

Joao-L-S-Almeida commented Sep 5, 2024

myscon commented Sep 5, 2024

Joao-L-S-Almeida commented Sep 6, 2024

myscon commented Sep 6, 2024

Joao-L-S-Almeida commented Sep 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CarlosGomes98 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment