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

float type conversion #146

Closed
zvikam opened this issue Feb 27, 2018 · 5 comments
Closed

float type conversion #146

zvikam opened this issue Feb 27, 2018 · 5 comments

Comments

@zvikam
Copy link

zvikam commented Feb 27, 2018

Hi,
Was there a particular reason you decided to do the float64->float32 conversion (other than the obvious memory consumption) ?

Can you think of a way to make this conversion optional?

@daavoo
Copy link
Owner

daavoo commented Feb 28, 2018

Hello!

May I know what trouble is causing you this conversion?

As far as I remember, float64 caused the export to binary formats (i.e ply) to fail.

That was the main reason to force the conversion in PyntCloud.to_file BUT I don't remember why the conversion is also forced every time PyntCloud.points is updated.

I think that the conversion could be easily converted to optional at instantiation.

For example, you could add an attribute to be setted in PyntCloud.__init__ (i.e force_float32=False) and then access that attribute in all places where the conversion happens:

if self.force_float32:
    convert_columns_dtype(self.points, np.float64, np.float32)

That is just one idea.

I would definitely look into why the conversion happens in other places than to_file.

@zvikam
Copy link
Author

zvikam commented Feb 28, 2018

Hi,
I found your package and wanted to check if I could replace my current PLY package, and then I encountered inconsistencies during testing because some of my existing PLY files store (binary) float64 values.

Your suggestion for an attribute was along the lines of what I had in mind, too.

I'll continue my testing and see if I can figure out the float64 problem in case I encounter it.
I'll also check if my application can tolerate moving to float32 values in the PLY file.

@daavoo
Copy link
Owner

daavoo commented Feb 28, 2018

The problem I found was that even though I could export/import PLY files with float64 values inside pyntcloud, those files weren't correctly loaded in other programs such as Three.js or CloudCompare so I just decided to force the float32 conversion because any of the logic in pyntcloud was affected by that.

@zvikam
Copy link
Author

zvikam commented Feb 28, 2018

Ok, so I guess my best option is to implement a conditional conversion (default: enabled).

@daavoo
Copy link
Owner

daavoo commented Apr 5, 2018

I was reviewing the code base and I think that the dtype conversion can be moved to the io methods, where it's required, so it will be possible to work with float64 columns.

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

2 participants