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

feat(structuredgrid): add method to to get ij from point(s) in real world coordinates #1205

Closed
wants to merge 1 commit into from

Conversation

ntdosch
Copy link

@ntdosch ntdosch commented Aug 20, 2021

The get_ij method returns the row and column of a point or sequence of points in real-world coordinates. It's is effectively the same as the method by the same name in the now depreciated SpatialReference class.

…in real world coords. Same as the function of the same name in the now depreciated SpatialReference class.
@jdhughes-usgs
Copy link
Contributor

@ntdosch before we can accept a PR with new functionality you need to add add one or more tests. You could add it to an existing autotest script (for example, t031_test.py).

@jlarsen-usgs It seems it would be better to have a similar method for each discretization type.

A proposal would be:

cellid = modelgrid.get_cellid(x, y, z=None),

which would return (i, j) for StructuredGrid, (cell2d) for VertexGrid, and None for UnstructuredGrid, and

cellid = modelgrid.get_cellid(x, y, z=z)

which would return (k, i, j) for StructuredGrid, (k, cell2d) for VertexGrid, and (node) for UnstructuredGrid.

You could start with implementing it in StructuredGrid and add the base method in Grid and issue a NotImplementedError exception in the base method.

Not sure if you are interested in implementing something this general but we would appreciate the help.

@jdhughes-usgs
Copy link
Contributor

@ntdosch and @jlarsen-usgs I forgot the modelgrid.intersect(x, y) does what get_ij() in SpatialReference did.

So this current PR duplicates functionality. May be worthwhile to add an optional z coordinate to intersect(), which would allow the method to be implemented for UnstructuredGrid.

@jlarsen-usgs what do you think. If it seems reasonable we can leave the PR open and tag it as an enhancement for some future version.

@jlarsen-usgs
Copy link
Contributor

@jdhughes-usgs

I think adding an optional z parameter to intersect() would be resonable and shouldn't be too difficult to implement.

@jdhughes-usgs
Copy link
Contributor

closing PR since this method already exists as i, j = modelgrid.intersect(x, y). Opened issue #1208 to enhance .intersect() method to take an option z argument.

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

Successfully merging this pull request may close these issues.

3 participants