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

changed to using decimal for conversion #720

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

fcollman
Copy link
Contributor

Here's a suggestion for how to resolve #719, not sure if i properly anticipated all the problems this might cause.

@fcollman
Copy link
Contributor Author

@jbms any comments on whether this approach seems ok?

@jbms
Copy link
Collaborator

jbms commented Feb 25, 2025

On the one hand, it seems always better to avoid losing precision when possible.

However, Neuroglancer itself uses floating point operations and therefore I think it is just chance that it happened to work here. In particular, while Neuroglancer happens to get the scale of the z dimension as 0.00003m exactly, it ends up with a scale of 6.500000000000001e-7 for x and y.

I created a separate fix of the underlying "instability" in Neuroglancer: #742

With that PR, things work as expected even with the mismatch.

There might still be some room to improve things on the Python side to make the behavior more consistent with Neuroglancer. But I'm not sure that using infinite precision to do the calculations is the answer.

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.

floating point division creates mistakes in resolution when using non m units
2 participants