Initial Implementation of Latitude Longitude Face Bounds

[philipc2]

Closes #91

Overview

• Introduces the Grid.bounds variable, which contains the Latitude-Longitude bounds for each face.

Expected Usage

import uxarray as ux

grid_path = "/path/to/grid.nc"

uxgrid = ux.open_grid(grid_path)

# obtain bounds
uxgrid.bounds

[philipc2]

Some timing benchmarks on a few of the grids that we have in our test suite.

The GeoFlow grid (12s execution time) has 6000 nodes and 3840 faces.

I'll continue investigating optimizations into the performance, however all things considered we can begin to move forward with getting this PR merged (once I finalize a few review comments)

[erogluorhan]

I did a first pass over the code. Thanks for this!

Re @philipc2's benchmarking:

The GeoFlow grid (12s execution time) has 6000 nodes and 3840 faces.

That's extremely slow for such a small grid (though relatable as the algorithm iterates over each face, and each edge of that face). Taking into account bigger grids might be used, the execution times might make the things impractical, and that makes me think that we should consider either to optimize before a release, or provide caution admonitions, if possible, in the documentation to let the user clearly know the extreme performance bottleneck.

Thoughts?

[philipc2]

That's extremely slow for such a small grid (though relatable as the algorithm iterates over each face, and each edge of that face). Taking into account bigger grids might be used, the execution times might make the things impractical, and that makes me think that we should consider either to optimize before a release, or provide caution admonitions, if possible, in the documentation to let the user clearly know the extreme performance bottleneck.

Can you add this note to #730?

[philipc2]

Pinging @hongyuchen1030

Made a couple changes, the performance issues will be handled with #730. Once the above comments are addressed, I'm good with merging it.

[philipc2]

@hongyuchen1030

Maybe for time being, we can make bounds an internal attribute while we work on optimizing the perfromance.

Grid._bounds

This would allow you to continue working while we work on the optimization. Thoughts?

[hongyuchen1030]

@hongyuchen1030

Maybe for time being, we can make bounds an internal attribute while we work on optimizing the perfromance.

Grid._bounds

This would allow you to continue working while we work on the optimization. Thoughts?

I am open to any suggestion as long as I can continue my work. The main point here is: Aren't we supposed to give users at least something they can use now? I know Paul has been receiving numerous requests for these functionalities and keeps asking if there's anything they can start using right now. I feel like at this point doing an agile development might be better for users than a cascade development. We can always put a warning/ description for this function.

We can also discuss this in our next Uxarray meeting as well.

[erogluorhan]

I am open to any suggestion as long as I can continue my work. The main point here is: Aren't we supposed to give users at least something they can use now? I know Paul has been receiving numerous requests for these functionalities and keeps asking if there's anything they can start using right now. I feel like at this point doing an agile development might be better for users than a cascade development. We can always put a warning/ description for this function.

We can also discuss this in our next Uxarray meeting as well.

12s execution time for a pretty small grid with only 3840 faces is raising a concern about its usability.

I feel like either (1) making it internal until we make usable optimization (for instance, we were able to optimize the nodal averaging over the hackathon by around 80x by only Numpy vectorization), or (2) displaying a strong caution about its performance when the user attempts to use it would suffice for now. We can get your work going on with either of these as well.

I lean towards the option one personally but am happy to discuss. If you'll have a chance to meet with Paul before the next week's meeting, please feel free to discuss before then, bringing these comments up. Otherwise, let's discuss next week.

[philipc2]

@hongyuchen1030
Maybe for time being, we can make bounds an internal attribute while we work on optimizing the perfromance.

Grid._bounds

This would allow you to continue working while we work on the optimization. Thoughts?

I am open to any suggestion as long as I can continue my work. The main point here is: Aren't we supposed to give users at least something they can use now? I know Paul has been receiving numerous requests for these functionalities and keeps asking if there's anything they can start using right now. I feel like at this point doing an agile development might be better for users than a cascade development. We can always put a warning/ description for this function.

We can also discuss this in our next Uxarray meeting as well.

That's a fair point. Let's keep it as part of the user API (i.e. Grid.bounds) with a warning within the docstring.

Another data point, I ran this on a 82k node / 41k face mesh and it ran in ~3 minutes, which for this initial implementation is acceptable.

[philipc2]

I will be adding a warning about the performance. Once all comments are addressed, we will get it merged.