Finodev #152

ashiq24 · 2023-06-07T22:28:40Z

No description provided.

…dding layers. Newly added modules attention, fourier kernel integration

…nd-to-end scaling factor from layerwise scaling factors

JeanKossaifi

Thanks @ashiq24, left some comments

JeanKossaifi · 2023-06-10T00:21:25Z

neuralop/models/uno.py

        if isinstance(self.output_scaling_factor, (float, int)):
            self.output_scaling_factor = [self.output_scaling_factor]*self.n_dim

-
+        print("calculated out factor", self.output_scaling_factor)


Let's add an attribute verbose (pass it in init) and only print if self.verbose:

JeanKossaifi · 2023-06-10T00:21:37Z

neuralop/models/uno.py

@@ -195,7 +201,7 @@ def __init__(self,
                                            use_mlp=use_mlp, 
                                            mlp_dropout=mlp_dropout, 
                                            mlp_expansion=mlp_expansion,
-                                            output_scaling_factor = self.uno_scalings[i],
+                                            output_scaling_factor = [self.uno_scalings[i]],


Why is it a list?

The Fno blocks takes scaling_factors which are 2d list
Scaling factor for each layer and scaling factor along each dims.
It can also take a scaler and also 1D list with scaling factor along each layer.

JeanKossaifi · 2023-06-10T00:21:52Z

neuralop/models/uno.py

@@ -186,7 +192,7 @@ def __init__(self,

            if i in self.horizontal_skips_map.keys():
                prev_out = prev_out + self.uno_out_channels[self.horizontal_skips_map[i]]
-            print([self.uno_scalings[i]])
+            print(self.uno_scalings[i])


I don't think this print is necessary

removed it.

JeanKossaifi · 2023-06-10T00:22:04Z

neuralop/models/uno.py

@@ -225,6 +231,7 @@ def forward(self, x):

        if self.domain_padding is not None:
            x = self.domain_padding.pad(x)
+        print(x.shape)


Let's remove this and the next print too

JeanKossaifi · 2023-06-10T00:22:26Z

neuralop/models/uno.py

        self.operator_block = operator_block
        self.integral_operator = integral_operator

        if self.horizontal_skips_map is None:
            self.horizontal_skips_map = {}
            for i in range(n_layers//2,0,):
                self.horizontal_skips_map[n_layers - i -1] = i
-
+
+        self.output_scaling_factor = np.ones_like(self.uno_scalings[0])


Let's explicitly make this a list, not a tensor

@ashiq24 -- a list would be better here

JeanKossaifi · 2023-06-10T00:22:46Z

neuralop/models/uno.py

-
+
+        self.output_scaling_factor = np.ones_like(self.uno_scalings[0])
+        for k in self.uno_scalings:


Can you add some comment here, it's hard to read and understand the logic.

Let's add some comments before merging.

JeanKossaifi · 2023-06-10T00:23:55Z

neuralop/models/uno.py

+        self.output_scaling_factor = np.ones_like(self.uno_scalings[0])
+        for k in self.uno_scalings:
+            self.output_scaling_factor = np.multiply(self.output_scaling_factor, k)
+        self.output_scaling_factor = self.output_scaling_factor.tolist()


Let's make it a list and not add additional tolist.
You can replace the np.multiply (which should work on a list anyway) with math.prod.
Actually we don't need the list, you can directly take the prod I think

math.prod multiply numbers in a list. but I need to multiply 2 lists. Each list specifies the scaling factors along each of the dims.

Hmm this code is becoming very confusing - can we try to think and simplify this scaling API?

To do the scaling we need two things ---
one -- scaling factor for each layer.
two -- scaling factor along each of the dimension.
We need a two array in general.
But in case, we scale each of the dimensions with same scaling factor, then it can be a 1d array, where each element is the scaling factor for each layer.

Alright, let's process it to always be a nested list, at the highest level and we can use zip to do the element-wise multiplication.

Do we need to have both output_scaling_factor and uno_scaling?

JeanKossaifi · 2023-06-10T00:25:27Z

neuralop/models/spectral_convolution.py

@@ -480,8 +480,8 @@ def __init__(self, in_channels, out_channels, n_modes, incremental_n_modes=None,

        hm1, hm2 = self.half_n_modes[0], self.half_n_modes[1]
        if self.factorization is None:
-            self.W1 = nn.Parameter(torch.empty(self.in_channels, hm1, hm2, hm1, hm2, dtype = torch.cfloat))
-            self.W2 = nn.Parameter(torch.empty(self.in_channels, hm1, hm2, hm1, hm2, dtype = torch.cfloat))
+            self.W1 = nn.Parameter(torch.empty(hm1, hm2, hm1, hm2, dtype = torch.cfloat))


Let's add this in a separate PR

JeanKossaifi · 2023-06-10T00:25:56Z

neuralop/models/padding.py

            output_pad = [int(round(i*j)) for (i,j) in zip(self.output_scale_factor,output_pad)]

            if self.padding_mode == 'symmetric':
                # Pad both sides
-                unpad_indices = (Ellipsis, ) + tuple([slice(p, -p, None) for p in output_pad ])
+                unpad_indices = (Ellipsis, ) + tuple([slice(p, -p, None) for p in output_pad[::-1] ])


Why did you reverse here?

The first element of the output_pad pads around the last dimension. So, the first element should in the last slice.

Why is that the case? Shouldn't it be in the same order?

As per my understanding from the documentation, it is (left, right, top, botton, front, back) for 3D tensor... Left/right padding pads along the dim = -1. Top/bottom pads along the dim = -2... and so on.

Great point @ashiq24 - can we make a quick comment and point to the doc for this?

You meant in the code as a doc string?

Yes, in the code just with # comment :)

@ashiq24 can you add a short comment here?

JeanKossaifi · 2023-06-10T00:26:08Z

neuralop/models/resample.py

+#     if len(axis) == 1:
+#         return F.interpolate(x, size = new_size[0], mode = 'linear', align_corners = True)
+#     if len(axis) == 2:
+#         return F.interpolate(x, size = new_size, mode = 'bicubic', align_corners = True, antialias = True)


Why did you comment these?

Can we update to still use them, they are faster

I can update this. But then the neural operator can only be used with PyTorch version later than 1.9.1. Because, " antialias" is not available in the previous distribution of PyTorch.

Makes sense, let's open an issue to address this in a separate PR.

…nto finodev

JeanKossaifi · 2023-06-12T18:53:54Z

.gitignore

@@ -79,3 +79,5 @@ target/

 #Ipython Notebook
 .ipynb_checkpoints
+
+setup_git.sh


I don't think this needs to be in the main repo

Let's remove this one.

JeanKossaifi · 2023-06-12T18:54:50Z

Thanks @ashiq24 looks good overall but looks like the latest PR merged created a conflict, can we resolve?

ashiq24 · 2023-06-16T18:43:39Z

I think it now suitable for merging.

JeanKossaifi

Looks good overall, let's just address the comments to make the code a little clearer before merging.

JeanKossaifi · 2023-06-21T02:19:15Z

neuralop/models/padding.py

            output_pad = [int(round(i*j)) for (i,j) in zip(self.output_scale_factor,output_pad)]

            if self.padding_mode == 'symmetric':
                # Pad both sides
-                unpad_indices = (Ellipsis, ) + tuple([slice(p, -p, None) for p in output_pad ])
+                unpad_indices = (Ellipsis, ) + tuple([slice(p, -p, None) for p in output_pad[::-1] ])


@ashiq24 can you add a short comment here?

JeanKossaifi · 2023-06-21T02:19:23Z

.gitignore

@@ -79,3 +79,5 @@ target/

 #Ipython Notebook
 .ipynb_checkpoints
+
+setup_git.sh


Let's remove this one.

JeanKossaifi · 2023-06-21T02:20:08Z

neuralop/models/resample.py

+#     if len(axis) == 1:
+#         return F.interpolate(x, size = new_size[0], mode = 'linear', align_corners = True)
+#     if len(axis) == 2:
+#         return F.interpolate(x, size = new_size, mode = 'bicubic', align_corners = True, antialias = True)


Makes sense, let's open an issue to address this in a separate PR.

JeanKossaifi · 2023-06-21T02:20:32Z

neuralop/models/uno.py

        self.operator_block = operator_block
        self.integral_operator = integral_operator

        if self.horizontal_skips_map is None:
            self.horizontal_skips_map = {}
            for i in range(n_layers//2,0,):
                self.horizontal_skips_map[n_layers - i -1] = i
-
+
+        self.output_scaling_factor = np.ones_like(self.uno_scalings[0])


@ashiq24 -- a list would be better here

JeanKossaifi · 2023-06-21T02:20:42Z

neuralop/models/uno.py

-
+
+        self.output_scaling_factor = np.ones_like(self.uno_scalings[0])
+        for k in self.uno_scalings:


Let's add some comments before merging.

JeanKossaifi · 2023-06-21T02:20:49Z

neuralop/models/uno.py

+        self.output_scaling_factor = np.ones_like(self.uno_scalings[0])
+        for k in self.uno_scalings:
+            self.output_scaling_factor = np.multiply(self.output_scaling_factor, k)
+        self.output_scaling_factor = self.output_scaling_factor.tolist()


…nto finodev

JeanKossaifi · 2023-06-23T02:10:22Z

Thanks @ashiq24, it looks good - just one last thing, can you have a look at pep8?
In particular, can you remove the spaces around the =, at least in the function calls/signatures?
Good to merge aside from that.

…nto finodev

…le to make it runanble even when the device is set to cuda

JeanKossaifi · 2023-06-26T17:55:51Z

Thanks @ashiq24, merging.

* changed modules are: UNO, FNOblock, factorizedspcetralconvolution, padding layers. * fixed the padding function, more test cases for UNO test, calculate end-to-end scaling factor from layerwise scaling factors * Update padding.py with documentation about F.pad Authored-by: ashiq24 <ashuiqbuet14@gmail.com>

ashiq24 and others added 6 commits June 7, 2023 18:27

fino_dev

37f18cb

problem

c1f8be4

mag k

9c443c9

changed modules are: UNO, FNOblock, factorizedspcetralconvolution, pa…

ad609e1

…dding layers. Newly added modules attention, fourier kernel integration

fixed the padding function, more test cases for UNO test, calculate e…

8219e48

…nd-to-end scaling factor from layerwise scaling factors

Merge branch 'main' into finodev

aa1f1de

JeanKossaifi reviewed Jun 10, 2023

View reviewed changes

ashiq24 and others added 6 commits June 10, 2023 00:29

removed transformer related dev

59950b4

Merge branch 'finodev' of https://github.com/ashiq24/neuraloperator i…

2ad0920

…nto finodev

removed transformer related dev

0f2dedc

removed transformer related dev

8a749b6

addressing comments. Removed print statements

5f1f1bc

Update padding.py with documentation about F.pad

53fa69c

JeanKossaifi reviewed Jun 12, 2023

View reviewed changes

ashiq24 and others added 3 commits June 14, 2023 16:42

Merge branch 'main' into finodev

ce9e524

merge_reqest_problem

a8a9ab6

merge_reqest_problem

73b9f92

ashiq24 requested a review from JeanKossaifi June 16, 2023 18:42

ashiq24 added 2 commits June 20, 2023 22:09

msg

4c97ca4

msg

1b12f4c

JeanKossaifi reviewed Jun 21, 2023

View reviewed changes

ashiq24 and others added 7 commits June 21, 2023 20:35

comments added

f622e58

Updated .gitignore

5421cf2

comments added

f9263fd

Merge branch 'finodev' of https://github.com/ashiq24/neuraloperator i…

57a2236

…nto finodev

comments added

a84bcf4

Update .gitignore

5fb7528

last_multiplication_and_fino_removed

d008335

ashiq24 added 4 commits June 23, 2023 23:16

msg

239bb49

Merge branch 'finodev' of https://github.com/ashiq24/neuraloperator i…

df6b9f4

…nto finodev

addressed some pep8 issues. Also added very minor change to Uno examp…

c8f1ea3

…le to make it runanble even when the device is set to cuda

Update .gitignore

6e962c9

JeanKossaifi merged commit 1aefe93 into neuraloperator:main Jun 26, 2023
1 check passed

ashiq24 deleted the finodev branch July 22, 2023 00:51



		self.output_scaling_factor = np.ones_like(self.uno_scalings[0])
		for k in self.uno_scalings:

Finodev #152

Finodev #152

Conversation

ashiq24 commented Jun 7, 2023

JeanKossaifi 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

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

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

JeanKossaifi commented Jun 12, 2023

ashiq24 commented Jun 16, 2023

JeanKossaifi 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

JeanKossaifi commented Jun 23, 2023

JeanKossaifi commented Jun 26, 2023