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

Changing GVRS API, need comments #14

Closed
gwlucastrig opened this issue Feb 10, 2022 · 1 comment
Closed

Changing GVRS API, need comments #14

gwlucastrig opened this issue Feb 10, 2022 · 1 comment

Comments

@gwlucastrig
Copy link
Owner

As I was writing the "getting started" pages for the Gridfour wiki, I realized that I'd made a mistake in the way I created some of the coordinate-transform methods in the GVRS API.

As a solution, I am preparing for release 1.0.1 in the near future.

I'd like to get some user comments on this issue. In particular, I'd like to get comments on my proposed approach to making this change. I would prefer to remove the old methods and replace them with the new. Ordinarily, my approach to this issue would be to deprecate the old methods and remove them in the future. But since GVRS is so new, and so few users have adopted it (so far), I think it would be cleaner to just make the change. So I would like to get some user feedback on how much of an inconvenience this would be.

The Proposed Change

The existing code includes methods in the form:

double []gridPoint = spec.mapGeographicToGrid(latitude, longitude);
double []geoPoint  = spec.mapGridToGeographic(gridPoint[0], gridPoint[1];

I think that the use of arrays was a mistake. First off, they are error prone. It's easy to get confused as
to which array index refers to which coordinate. Is geoPoint[0] a latitude or a longitude? Unless you know the API pretty well, you may be unsure.

I propose to replace these methods with an alternate approach:

GvrsGridPoint gridPoint = spec.mapGeographicToGridPoint(latitude, longitude);
GvrsGeoPoint  geoPoint  = spec.mapGridToGeoPoint(gridPoint);
System.out.println("row:    "+gridPoint.getRow());
System.out.println("column: "+gridPoint.getColumn());

I've tested these changes for performance and the new class-based technique is only a tiny bit slower than the array based approach (on average only about 1 part in 2397). In fact, in 18 our of 40 trials, the class-based technique was actually faster than the array-based form. So any differences are probably in the noise.

@gwlucastrig
Copy link
Owner Author

Updates were added for release 1.0.1. Old-style methods were deprecated, but retained.

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

No branches or pull requests

1 participant