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

Adding target_dim as parameter to .transform() #642

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

NoraLoose
Copy link

@NoraLoose NoraLoose commented Oct 11, 2024

Still WIP

I added a section to the existing doc/transform.ipynb notebook to document the new capability. While working on this notebook, I also fixed pre-existing issues with this notebook including:

  • Insert autoparse_metadata=False into Grid() to avoid error messages
  • Correct dictionary keys for pangeo cloud data library
  • Remove non-existing xc coordinate from Grid object to avoid existing ValueError
  • Insert a .squeeze() for plotting AMOC to avoid error

- Insert autoparse_metadata=False into Grid() to avoid error messages
- Correct dictionary keys for pangeo cloud data library
- Remove non-existing xc coordinate from Grid object to avoid
  existing ValueError
- Insert a .squeeze() for plotting AMOC to avoid error
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@NoraLoose NoraLoose changed the title Transform target dim Adding target_dim as parameter to .transform() Oct 11, 2024
@NoraLoose NoraLoose marked this pull request as draft October 11, 2024 21:31
@NoraLoose
Copy link
Author

NoraLoose commented Oct 11, 2024

The changes made in this PR are not the reason for the failing checks above. The same checks fail in #641. I opened issues #643 and #644.

@NoraLoose
Copy link
Author

Just realized that there is overlap between this PR and PR #627

@jbusecke
Copy link
Contributor

Hey @NoraLoose, thank you so much for working on this and sorry for the delay. I think I got the CI issues #644 and #648 under control now. Once #649 is merged can you try to rebase this one?

Just realized that there is overlap between this PR and PR #627

Feel free to absorb and close this if needed.

- Adapt construction of test data to multi-dimensional data
- Add linear multidimensional test cases to dictionary
- Add new test that tests that transforming onto spatially
  varying vertical target coordinate works as expected
@jbusecke
Copy link
Contributor

Also I have restructured the linting and setup in #650, please let me know if it is ok to merge this ahead of time. I think this should not cause too big of a conflict?

@NoraLoose
Copy link
Author

Also I have restructured the linting and setup in #650, please let me know if it is ok to merge this ahead of time. I think this should not cause too big of a conflict?

Yep, feel free to merge #650!

@NoraLoose NoraLoose marked this pull request as ready for review October 17, 2024 15:59
@NoraLoose
Copy link
Author

Transforming onto a spatially varying vertical coordinate (e.g., terrain-following) works with the linear interpolation method, see newly added tests and the example in the documentation.

I tested this also for the conservative interpolation method by adding the following test case to the cases dictionary in xgcm/test/test_transform.py:

    "conservative_depth_depth_multidim_target": {
        "source_coord": ("z", [5, 25, 60]),
        "source_bounds_coord": ("zc", [0, 10, 50, 75]),
        "source_data": ("data", [1, 4, 0]),
        "source_additional_data_coord": (
            "zc",
            [0, 10, 50, 75],
        ),
        "source_additional_data": ("zc", [0, 10, 50, 75]),
        # 2D target
        "target_dims": ("eta_rho", "s_w"),  # eta_rho: horizontal dimension; s_w: vertical dimension
        "target_coord": ("interface_depth_rho", np.array([[0, 1, 10, 50, 80], [0, 5, 20, 30, 100]])),
        "target_data": ("interface_depth_rho", np.array([[0, 1, 10, 50, 80], [0, 5, 20, 30, 100]])),
        "expected_dims": ("eta_rho", "s_w"),
        "expected_coord": ("interface_depth_rho", np.array([[0.5, 5.5, 30, 65], [2.5, 12.5, 25, 65]])),
        "expected_data": (
            "data",
            np.array([[0.1, 0.9, 4.0, 0.0], [0.5, 1.5, 1.0, 2.0]])
        ),
        "grid_kwargs": {
            "coords": {"Z": {"center": "z", "outer": "zc"}},
            "autoparse_metadata": False,
        },
        "transform_kwargs": {"method": "conservative", "target_data": "zc", "target_dim": "s_w"},
    }

However, it fails because of this line:

assert target_theta_bins.ndim == 1

@NoraLoose
Copy link
Author

@jbusecke I'm not sure when I have time to look into generalizing the conservative method to multiple dimensions. We could merge this PR, and open a feature request issue? If we do this, should we include more warnings (where?) to clarify that the conservative method is not yet supported for multi-dimensional vertical target coordinates?

@jbusecke
Copy link
Contributor

Thank you so much @NoraLoose! This is awesome. I am pretty busy today, but will try to take some time next week to review and maybe also enable conservative interpolation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants