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

replace _mul function with explicit casts #3143

Merged
merged 2 commits into from
Jun 1, 2021
Merged

replace _mul function with explicit casts #3143

merged 2 commits into from
Jun 1, 2021

Conversation

mpenkov
Copy link
Collaborator

@mpenkov mpenkov commented May 15, 2021

Follow up from #3136

@mpenkov mpenkov requested review from gojomo and piskvorky May 15, 2021 09:24
@piskvorky
Copy link
Owner

piskvorky commented May 18, 2021

@mpenkov can you also remove the (empty) file 16? Added in #3136, probably by mistake.

@gojomo
Copy link
Collaborator

gojomo commented May 18, 2021

I personally find this inline, direct approach clearer - so, I prefer it to the _mul() function approach.

But also, I'd suggested previously a cdef/ctypedef might possibly make it even more compact. Thinking more about it, it strikes me that best of all could be to explicitly reuse an existing standard numpy type. For example, replacing <long long> (which may still have risks of varying meaning across platforms) with <np.uint64_t> might best both communicate intent & also ensure uniform behavior across our supported platforms. (If it breaks, it'll break the same everywhere these same numpy arrays are being used, which is preferable.)

@mpenkov mpenkov merged commit f3f6d2d into develop Jun 1, 2021
@mpenkov mpenkov deleted the post_3136 branch June 1, 2021 01:45
@gojomo
Copy link
Collaborator

gojomo commented Jun 1, 2021

Would the more-explicit/more-portable <np.uint64_t> not have worked?

@piskvorky
Copy link
Owner

I like the explicit width version too.

Writing the comment seems comparable effort to writing the commit – could you help @mpenkov out here? Thanks.

@gojomo
Copy link
Collaborator

gojomo commented Jun 1, 2021

The change would best be done by someone set up to verify it fixes the test case, which I won't be set up to do anytime soon.

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.

3 participants