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.

Sign up for GitHub

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

Jump to bottom

Log coordinates as integers #268

Merged
merged 2 commits into from
Oct 25, 2017
Merged

Log coordinates as integers #268

merged 2 commits into from
Oct 25, 2017

Conversation

rmarianski
Copy link
Member

@rmarianski rmarianski commented Oct 24, 2017 •
edited
Loading

@zerebubuth
Copy link
Member

Is this intended to trim trailing zeroes, e.g: print out as {"z": 3, "x": 4, "y": 5} rather than {"z": 3.0, "x": 4.0, "y": 5.0}?

I'm fairly sure that, in some circumstances, we might have fractional coordinates and it would be good to know about that when they're printed out. Perhaps we could integerise if the coordinate is within an ulp of being an integer already?

@rmarianski
Copy link
Member Author

Is this intended to trim trailing zeroes, e.g: print out as {"z": 3, "x": 4, "y": 5} rather than {"z": 3.0, "x": 4.0, "y": 5.0}?

Yes, exactly this.

I'm fairly sure that, in some circumstances, we might have fractional coordinates and it would be good to know about that when they're printed out. Perhaps we could integerise if the coordinate is within an ulp of being an integer already?

Really, when? Happy to just coerce to int if it's an even integer, but just curious when we would come across this scenario.

@zerebubuth
Copy link
Member

Really, when? Happy to just coerce to int if it's an even integer, but just curious when we would come across this scenario.

The row and column of a Coordinate can be fractional if we zoom out and forget to call .container(). I can't think of an occasion where we do that deliberately, so perhaps the most likely source is a bug. But if it is a bug, and it's causing problems, then I think it would be much easier to notice if it's logged out as fractional.

@rmarianski
Log integers conditionally
972c227 
We should expect that the coordinate values always be integers, but if
we ever call container and forget to truncate at that point, it's
probably a bug and we'd prefer to log that instead.
@rmarianski
Copy link
Member Author

Ah ok, that's fair. Updated in 972c227.

@rmarianski
Copy link
Member Author

Assuming this is ok now and merging in. Let me know if there's some further follow up.

@rmarianski rmarianski merged commit b9bb785 into master Oct 25, 2017
@rmarianski rmarianski deleted the log-coord-int branch October 25, 2017 13:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iandees iandees Awaiting requested review from iandees

@zerebubuth zerebubuth Awaiting requested review from zerebubuth

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rmarianski @zerebubuth