Skip to content
This repository was archived by the owner on Nov 30, 2023. It is now read-only.

Corrected order of operations. Made runnable.#4

Closed
dsoprea wants to merge 2 commits intogoogle:masterfrom
dsoprea:master
Closed

Corrected order of operations. Made runnable.#4
dsoprea wants to merge 2 commits intogoogle:masterfrom
dsoprea:master

Conversation

@dsoprea
Copy link

@dsoprea dsoprea commented Jan 20, 2017

This is a pretty petty PR, but the order of the operations in the example in the README is non-sensical. I flipped them and also restructured the example so that it was copy-and-paste executable (including defining x and y).

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@dsoprea
Copy link
Author

dsoprea commented Jan 20, 2017

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

@bramp
Copy link
Contributor

bramp commented Jan 20, 2017

Thanks for making it compilable, but why reverse the order of Map and MapInverse? I think it reads better if you went

    // Example value.
    t := 144 

    // Now map one dimension numbers in the range [0, N*N-1], to an x,y
    // coordinate on the curve where both x and y are in the range [0, N-1].
    x, y, _ := s.Map(t)

    // x = 8
    // y = 12

    // Map parameters (x, y) back to t.
    t, _ := s.MapInverse(x, y)

    // t = 144

@dsoprea
Copy link
Author

dsoprea commented Jan 20, 2017

If you're referring to MapInverse occurring before Map then I agree with you in principle. However, t has considerably less meaning to people than does x and y (the coordinates; the only parameters that people will implicitly understand). By starting with an intentional value for t you're implying that the audience should implicitly understand this value. This might be confusing as t will usually be considered opaque by most. Of course, this is just my two cents. I can change it as suggested if you insist.

@coveralls
Copy link

coveralls commented Jan 20, 2017

Coverage Status

Coverage decreased (-45.5%) to 34.365% when pulling 822fb83 on dsoprea:master into 38d1238 on google:master.

@bramp
Copy link
Contributor

bramp commented Jan 20, 2017

I understand that x,y is more understandable than t. Sadly Wikipedia and others use different symbols for t, so there seems to be no less ambiguous letter. However, it looks backwards to me to have MapInverse before Map. The comment above the s.Map(t) hopefully explains we are mapping a single value, t, to a x,y coordinate.

@coveralls
Copy link

coveralls commented Jan 21, 2017

Coverage Status

Coverage decreased (-45.5%) to 34.365% when pulling 4b1e78c on dsoprea:master into 38d1238 on google:master.

@dsoprea
Copy link
Author

dsoprea commented Jan 21, 2017

Done. I just added the assertions. It was either that or printing out the values instead of just impotently putting the expected values in the comments (which started with me).

@bramp
Copy link
Contributor

bramp commented Jan 21, 2017

Now I'm wondering if perhaps it is better to use the existing example from the godoc: https://github.com/google/hilbert/blob/master/example_test.go#L23

Since this is early in the README the goal should be to focus on a quick example to get the point across. Having an example with error checking, and variables, just dilutes the example.

@coveralls
Copy link

coveralls commented Jan 29, 2017

Coverage Status

Coverage decreased (-36.4%) to 43.421% when pulling 7944a5c on dsoprea:master into 38d1238 on google:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-33.5%) to 46.309% when pulling 9cbd808 on dsoprea:master into 38d1238 on google:master.

3 similar comments
@coveralls
Copy link

Coverage Status

Coverage decreased (-33.5%) to 46.309% when pulling 9cbd808 on dsoprea:master into 38d1238 on google:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-33.5%) to 46.309% when pulling 9cbd808 on dsoprea:master into 38d1238 on google:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-33.5%) to 46.309% when pulling 9cbd808 on dsoprea:master into 38d1238 on google:master.

- Made example code runnable.
@coveralls
Copy link

Coverage Status

Coverage decreased (-33.5%) to 46.309% when pulling 2ad04e2 on dsoprea:master into 38d1238 on google:master.

1 similar comment
@coveralls
Copy link

coveralls commented Jan 29, 2017

Coverage Status

Coverage decreased (-33.5%) to 46.309% when pulling 2ad04e2 on dsoprea:master into 38d1238 on google:master.

@dsoprea
Copy link
Author

dsoprea commented Jan 29, 2017

Not completely sure why coverage dropped so much. It dropped 40% when I updated just the README, last time.

@bramp
Copy link
Contributor

bramp commented Jan 29, 2017

Would you add your 64 bit addition to it's own branch/pull request. And keep this PR for the README update. Also, quick comment, do you have a use case for the 64bit addition?

@dsoprea
Copy link
Author

dsoprea commented Jan 29, 2017

I can't keep the README changes here because I added comments to show the 64-bit support. Yes, I have a use-case. I'll open a new PR for the new branch and comment there.

@dsoprea dsoprea closed this Jan 29, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants