Skip to content
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

Application to 1-dim data #133

Open
dhruvbalwada opened this issue Feb 4, 2022 · 8 comments
Open

Application to 1-dim data #133

dhruvbalwada opened this issue Feb 4, 2022 · 8 comments
Labels
enhancement New feature or request kernels Related to low-level kernels question Further information is requested

Comments

@dhruvbalwada
Copy link

dhruvbalwada commented Feb 4, 2022

Is it possible to apply the package to 1D data? I noticed that there is an example in the paper applying to 1D satellite data (@jakesteinberg).
However, when I try to apply to a 1D data, I get the following error:

---------------------------------------------------------------------------
AssertionError                            Traceback (most recent call last)
Input In [45], in <module>
----> 1 test_data_filtered = filter_c.apply(test_data, dims=['XC',])

File ~/miniconda/envs/base_new/envs/GM-Redi-offline-env/lib/python3.10/site-packages/gcm_filters/filter.py:432, in Filter.apply(self, field_or_dataset, dims)
    430     return filtered
    431 else:
--> 432     return self.apply_to_field(field_or_dataset, dims=dims)

File ~/miniconda/envs/base_new/envs/GM-Redi-offline-env/lib/python3.10/site-packages/gcm_filters/filter.py:444, in Filter.apply_to_field(self, field, dims)
    442 filter_func = _create_filter_func(self.filter_spec, self.Laplacian)
    443 grid_args = [self.grid_ds[name] for name in self.Laplacian.required_grid_args()]
--> 444 assert len(dims) == 2
    445 n_args = 1 + len(grid_args)
    446 field_smooth = xr.apply_ufunc(
    447     filter_func,
    448     field,
   (...)
    453     dask="parallelized",
    454 )

AssertionError: 

Seems like 2-Dimensionality is asserted in the code. Am I missing something?

@dhruvbalwada dhruvbalwada added the question Further information is requested label Feb 4, 2022
@rabernat
Copy link
Contributor

rabernat commented Feb 4, 2022

No. 🙃

The method is applicable to 1D data. But this package assumes everywhere that the data are 2D. That could be changed, but that may be out of scope.

@iangrooms
Copy link
Member

I think all we need is a kernel for a 1D Laplacian. You can set ndim=1 in the Filter class, but we don't have a 1D Laplacian that goes with it.

@dhruvbalwada
Copy link
Author

Ah, I see. Is there a way to use the package to reproduce plots like Figure 11 in the paper? or was that done using code that is not here? Would creating a dimension of size 1 solve it?

@iangrooms
Copy link
Member

The code for that figure is not here, and it looks like it didn't make it onto the source repository for that paper. That said, all that would be needed to reproduce within gcm-filters it is to define a 1D Laplacian kernel. I think @jakesteinberg might also have the original code on one of his repositories.

@dhruvbalwada
Copy link
Author

Ok. Thank you for such a quick response. :)

@jakesteinberg
Copy link
Contributor

Hi! Yes, I feel late to the party haha. I used an older version of the code to make that figure (pre gcm-filters but with the same filter kernel idea/process, thanks to @iangrooms help way back at the beginning of this). I can help you make this work if you'd like. I guess I should also try and package up the 1D Laplacian kernel I have and make a pull request. The functions I built are in here and the main one 'Filter()' is applied to 1D data here. It is all a bit simpler (and messier) than gcm-filters, ha.

@dhruvbalwada
Copy link
Author

dhruvbalwada commented Feb 4, 2022

It is not something essential to me right now.
I was trying to test out gcm-filters, and thought doing it on periodic 1D data would be simplest case, when I ran into this. I just ended up stepping up 1 level and using 2D data.

However, thinking broadly I feel like this example should be within the scope of the package since the paper literally has a section titled "Application to 1-D observational data". Also if we want to use this package with most obs data, we will need it.

@iangrooms iangrooms added enhancement New feature or request kernels Related to low-level kernels labels Feb 4, 2022
@iangrooms
Copy link
Member

I agree that we should have this. It shouldn't be hard to convert existing 2D kernels to 1D, but we might want to add some extra errors/warnings. E.g. if the user tries to use a kernel that is not consistent with the value of ndim then the package should throw an error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request kernels Related to low-level kernels question Further information is requested
Projects
None yet
Development

No branches or pull requests

4 participants