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

Remove index shift by +-0.5 in voxel.py #388

Merged
merged 3 commits into from Apr 23, 2019

Conversation

wkentaro
Copy link
Contributor

@wkentaro wkentaro commented Apr 23, 2019

Close #370

NOTE: This is not backward compatible.

@mikedh
Copy link
Owner

mikedh commented Apr 23, 2019

Nice! This was sort of haphazard before, it's great to make it consistent. A few quick thoughts:

  • could we unit test?
  • are there other places do we do the conversion points->indices? We should probably be consistent
  • Maybe a quick blurb in the voxel.py top docstring which says something like "all voxels are referenced by the center of their cell box"

@wkentaro
Copy link
Contributor Author

wkentaro commented Apr 23, 2019

could we unit test?

I fixed the unit test just now.

are there other places do we do the conversion points->indices? We should probably be consistent

As far as I know, the conversion is only used in voxel.py file. So I think now is good.
The only concern is someone can assume Voxel.as_boxes returns boxes in the previous coordinate system. (origin is right-top of the first box)

Maybe a quick blurb in the voxel.py top docstring which says something like "all voxels are referenced by the center of their cell box"

I agree. I added a docstring for the Voxel class.

@coveralls
Copy link

coveralls commented Apr 23, 2019

Coverage Status

Coverage increased (+0.02%) to 84.002% when pulling 2d5e969 on wkentaro:remove_index_shift_voxel into dd0225c on mikedh:master.

@mikedh mikedh changed the base branch from master to merge/widget April 23, 2019 18:25
@mikedh
Copy link
Owner

mikedh commented Apr 23, 2019

Awesome, thanks!

@mikedh mikedh merged commit 360da4a into mikedh:merge/widget Apr 23, 2019
@wkentaro wkentaro deleted the remove_index_shift_voxel branch April 23, 2019 18:26
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

Successfully merging this pull request may close these issues.

None yet

3 participants