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

Remapping Notebook #807

Merged
merged 17 commits into from
Jun 25, 2024
Merged

Remapping Notebook #807

merged 17 commits into from
Jun 25, 2024

Conversation

aaronzedwick
Copy link
Member

@aaronzedwick aaronzedwick commented Jun 3, 2024

Closes #805

Overview

Adds a user guide section for remapping. This also updates the remapping functions to use face centers as default instead of nodes and from using kd_tree for cartesian to using ball_tree.

PR Checklist

General

  • An issue is linked created and linked
  • Add appropriate labels
  • Filled out Overview and Expected Usage (if applicable) sections

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

Copy link
Member

@philipc2 philipc2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have submitted a few comments under ReviewNB. Let me know if you can see them.

I also in Boulder this week, so I apologize if I'm a little slow on these reviews.

@aaronzedwick
Copy link
Member Author

I have submitted a few comments under ReviewNB. Let me know if you can see them.

I also in Boulder this week, so I apologize if I'm a little slow on these reviews.

That's understandable! Thanks for taking the time to review, I can't see any comments on ReviewNB though.

@philipc2
Copy link
Member

@aaronzedwick
Copy link
Member Author

Yes, thank you!

@philipc2
Copy link
Member

@@ -0,0 +1,557 @@
{
Copy link
Member

@philipc2 philipc2 Jun 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest cutting down the content a bit here and simplifying it to the following:

Remapping, or commonly referred to as Regridding, is the process of taking data that resides on one grid and mapping it to another. Details on various remapping methods can be found here. This user guide section will cover the two native remapping methods that are supported by UXarray:

  • Nearest Neighbor
  • Inverse Distance Weighted


Reply via ReviewNB

@@ -0,0 +1,557 @@
{
Copy link
Member

@philipc2 philipc2 Jun 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rephrase to the following:

In this notebook, we are using two datasets with different resolutions (480km and 120km) from the MPAS Ocean Model. We will be remapping the bottomDepth variable, which measures the ocean depth.


Reply via ReviewNB

@@ -0,0 +1,557 @@
{
Copy link
Member

@philipc2 philipc2 Jun 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rephrase:

We can view the supported remapping methods by accessing the .remap attribute that is part of a UxDataArray


Reply via ReviewNB

@@ -0,0 +1,557 @@
{
Copy link
Member

@philipc2 philipc2 Jun 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe mention somewhere that the Nearest Neighbor method is a distance-based approach, using an internal KD or Ball Tree to determine the distances?


Reply via ReviewNB

@@ -0,0 +1,557 @@
{
Copy link
Member

@philipc2 philipc2 Jun 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small change.

We can remap from the 480km grid to the 120km one, which would perform an upsampling operation.


Reply via ReviewNB

Copy link
Member

@philipc2 philipc2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a few more suggestions using ReviewNB.

Once we get a second set of eyes to proof read this, I think it'll be ready to go. This notebook is looking fantastic.

@@ -0,0 +1,557 @@
{
Copy link
Contributor

@rajeeja rajeeja Jun 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work and detailed description. Thanks

Possible to add a figure to explain the general remapping idea? two grids with a value mapped from one to another.


Reply via ReviewNB

@philipc2
Copy link
Member

Once #819 is merged, we need to update the references to destination_obj to destination_grid

@philipc2
Copy link
Member

Can you make sure to clear the output & restart the kernel for the notebook? Looks like some unnecessary outputs and metadata were included in the commit.

@@ -0,0 +1,1325 @@
{
Copy link
Member

@philipc2 philipc2 Jun 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets change up the order one last time to what we had before. We can move the import block back below the introductory paragraph, like you had before. Can you do the same for the other user guide notebooks as part of this PR too?

Should be

Title

Intro Paragraph

Imports

.. Rest of the nobook


Reply via ReviewNB

@@ -0,0 +1,1325 @@
{
Copy link
Member

@philipc2 philipc2 Jun 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a sub-title, something like

Simple Remapping Example


Reply via ReviewNB

@@ -0,0 +1,1325 @@
{
Copy link
Member

@philipc2 philipc2 Jun 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to make a very basic quad-triangle grid that you can use as the destination here, since remapping to the same grid could be a little confusing to the reader.

This example is great though! Very minimal and get the point across very nicely.


Reply via ReviewNB

@aaronzedwick
Copy link
Member Author

Can you make sure to clear the output & restart the kernel for the notebook? Looks like some unnecessary outputs and metadata were included in the commit.

Yeah, sorry about that, I forgot to hit save when I did that right before committing.

Copy link
Member

@philipc2 philipc2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fantastic work Aaron!

@philipc2 philipc2 merged commit 55e9011 into main Jun 25, 2024
15 checks passed
@aaronzedwick aaronzedwick deleted the zedwick/remapping_notebook branch June 26, 2024 20:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

User Guide Section on Remapping
3 participants