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

SF feature request #82 - Alternate type behaviour for xvals, yvals, zvals #330

Closed
mohawk2 opened this issue Jul 30, 2021 · 7 comments
Closed

Comments

@mohawk2
Copy link
Member

mohawk2 commented Jul 30, 2021

https://sourceforge.net/p/pdl/feature-requests/82/

@zmughal writes:
I noticed the caveat for xvals, yvals, and zvals regarding the type:

CAVEAT:

If you use the single argument piddle form (top row in the usage table) the
output will have the same type as the input; this may give surprising
results if, e.g., you have a byte array with a dimension of size greater
than 256. To force a type, use the third form
Wouldn't it make more sense to have the default one-argument version use the
PDL_IND datatype? I think this would be better as it would fit with the
principle of least astonishment.

Chris writes:
Makes sense to me. The PDL_Indx data type is a recent addition from the work to support 64bit indexing. Now that it exists, this change would be a logical update.

This seems like the correct thing to do but it would break back compatibility with existing usage. I'm not sure if the implicit type conversion would sufficiently hide the change to the API. Thoughts, anyone?

@mohawk2
Copy link
Member Author

mohawk2 commented Dec 26, 2021

A problem with this is that xlinvals uses xvals, and wants to basically do floating-point calculations from it. A better choice is almost certainly to have a minimum type (probably double) and upgrade the type to be that.

@zmughal
Copy link
Member

zmughal commented Dec 26, 2021

Using at least double would fix the caveat for most inputs since with 64-bit IEEE 754 double it would support [0, 2⁵³]. That would be a caveat, but it would have a smaller effect than the current one.

Perhaps use double for the existing xvals, etc. to avoid API changes and for optimization. If somebody needs it (so not now), it would be trivial to add an xvals_indx which does the same but with indx if only working with integers.

@karlglazebrook
Copy link
Member

karlglazebrook commented Dec 26, 2021 via email

@mohawk2
Copy link
Member Author

mohawk2 commented Jan 4, 2022

Fixing this is connected to Craig's thoughts on the sf.net origin for #217 (reshape behaviour inplace vs mv).

@mohawk2
Copy link
Member Author

mohawk2 commented Jan 7, 2022

As of f49ac06 xvals et al all return at least a double.

@djerius
Copy link
Contributor

djerius commented Jan 24, 2024

This change breaks code which requires xvals et al. to do what the documentation promised, namely return the type which was requested. That's a major API breakage (with no updates to the documentation)

It's now turning explicit requests for shorts and floats into doubles, which is definitely not what we want. This is doubling the memory requirements, and breaks long working code which passes ndarrays to C functions which require floats or shorts.

By all means make the default a double, but don't ignore a specific request for a type.

EDIT: Apologies for the tone. Long day...

@djerius djerius reopened this Jan 24, 2024
@djerius
Copy link
Contributor

djerius commented Jan 24, 2024

Sorry, here's some code:

$ perl -MPDL -E 'say xvals(short,2)->info'
PDL: Double D [2]

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

4 participants