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

Add function to get grid units #87

Open
mdpiper opened this issue Nov 4, 2021 · 9 comments · May be fixed by #99
Open

Add function to get grid units #87

mdpiper opened this issue Nov 4, 2021 · 9 comments · May be fixed by #99
Milestone

Comments

@mdpiper
Copy link
Member

mdpiper commented Nov 4, 2021

The BMI should have a function (or functions) to get the units of a grid.

@mdpiper
Copy link
Member Author

mdpiper commented Nov 4, 2021

As part of adding geospatial capabilities to BMI, it's suggested in #11 and #80 that BMI have a function (or functions) to do this. While this is needed for a grid with a coordinate reference system, it's generically useful, as well.

The inclusion of a grid unit getter function should be addressed separately from, and before, the addition of geospatial capabilities to BMI.

@mdpiper
Copy link
Member Author

mdpiper commented Nov 4, 2021

Suggested function prototypes, in SIDL, are here: #80 (comment).

@mcflugen
Copy link
Member

mcflugen commented Nov 4, 2021

To allow for different units for each coordinate, it seems like the new functions should be look like one of the following options.

Option 1

int get_grid_x_units(in int grid, out string units);
int get_grid_y_units(in int grid, out string units);
int get_grid_z_units(in int grid, out string units);

Pros:

  • easy to read
  • easy to implement

Cons:

  • adds three new functions
  • ties names (i.e. x, y, and z) to grid coordinates (but we already do that with get_grid_x, etc.)

Option 2

int get_grid_units(in int grid, out array<string, 1> units);

Pros:

  • adds only one additional function
  • names are not tied to grid coordinates (operates in the same way as get_grid_origin, get_grid_spacing, etc.)

Cons:

  • (slightly) harder to read
  • (slightly) harder to implement
  • returns units for all coordinates even if only one is needed

We could also add a dim argument that refers the dimension of the requested units,

int get_grid_units(in int grid, in int dim, out string units);

If we go with option 2, we may want to think about adding a get_grid_node_coordinates (to replace get_grid_[xyz]) function.

int get_grid_node_coordinates(in int grid, out array<double> coordinates)

or

int get_grid_node_coordinates(in int grid, in int dim, out array<double, 1> coordinates)

@mdpiper
Copy link
Member Author

mdpiper commented Nov 4, 2021

For Option 2, requiring the dim argument could simplify the call by making the units argument a string instead of a string array:

int get_grid_units(in int grid, in int dim, out string units);

@mcflugen
Copy link
Member

mcflugen commented Nov 4, 2021

@mdpiper Yes, of course. Good catch. I've updated my previous comment (so now it looks like you're slightly crazy by just restating what I just said).

@mdpiper
Copy link
Member Author

mdpiper commented Nov 4, 2021

Further, we could introduce another new function to get the names of the grid dimensions:

int get_grid_dimension_names(in int grid, out array<string, 1> names);

It could return ["x", "y", "z"] or ["x1", "x2", "x3"] or N names for grids with N dimensions.

Then, in get_grid_units, replace the dim parameter with name:

int get_grid_units(in int grid, in string name, out string units);

@mdpiper
Copy link
Member Author

mdpiper commented Nov 4, 2021

If we go with option 2, we may want to think about adding a get_grid_node_coordinates (to replace get_grid[xyz]_) function.

int get_grid_node_coordinates(in int grid, in int dim, out array<double, 1> coordinates)

Maybe we should wait for BMI 3 for this since it'll break backward compatibility.

@kettner
Copy link
Member

kettner commented Nov 9, 2021

Would a 4th dimension (time) be needed as well, so xyzt, or is this captured automatically?

@mdpiper
Copy link
Member Author

mdpiper commented Nov 9, 2021

Hey @kettner -- time is handled separately in the BMI through a set of time functions. We do need to represent grids of rank > 3, though; I'm making a separate issue for that now.

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

Successfully merging a pull request may close this issue.

3 participants