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

Closes #2472, #2800: Update IndexingMsg to use & instead of mod for setting bigint pdarrays #2793

Merged
merged 5 commits into from
Oct 6, 2023

Conversation

jaketrookman
Copy link
Contributor

@jaketrookman jaketrookman commented Sep 28, 2023

This PR (Closes #2472 and closes #2800) updates the function calls of mod in IndexingMsg to use %, as they are more efficient.
The bool case still needs to be updated as there is not a way to directly cast a bigint to a bool.

Leaving as a draft until I know if this last case can be completed without this casting feature.

@stress-tess
Copy link
Member

stress-tess commented Oct 3, 2023

@jaketrookman this isn't really what the issue was getting at

For this instead of using % or .mod, i was think we use &. This is what we do in other sections of code and it's more efficient. Basically instead of val % 2**max_bits we do val & ((2**max_bits)-1) so it functions as a bit mask instead

You can see something similar to this done in operatorMsg

var max_size = 1:bigint;
var has_max_bits = max_bits != -1;
if has_max_bits {
  max_size <<= max_bits;
  max_size -= 1;
}
...
if has_max_bits {
  li &= local_max_size;
}

Also just looking at this code statically, unless im misreading something what's already there seems to be incorrect. Because it's doing val % max_bits not val % 2**max_bits. Let me look into this a bit

@stress-tess
Copy link
Member

stress-tess commented Oct 3, 2023

yup the existing code (which i wrote 🙃 ) is incorrect when max_bits is set... see reproducer below. This is broken on master and is captured in issue #2800. We def want this corrected and tested before the next release

In [3]: bi = ak.arange(2**200 - 1, 2**200+4)

In [4]: bi
Out[4]: array([1606938044258990275541962092341162602522202993782792835301375 1606938044258990275541962092341162602522202993782792835301376 1606938044258990275541962092341162602522202993782792835301377 1606938044258990275541962092341162602522202993782792835301378 1606938044258990275541962092341162602522202993782792835301379])

In [5]: bi[:] = ak.arange(2**200 - 1, 2**200+4)

In [6]: bi
Out[6]: array([1606938044258990275541962092341162602522202993782792835301375 1606938044258990275541962092341162602522202993782792835301376 1606938044258990275541962092341162602522202993782792835301377 1606938044258990275541962092341162602522202993782792835301378 1606938044258990275541962092341162602522202993782792835301379])

In [7]: bi.max_bits = 201

In [8]: bi
Out[8]: array([1606938044258990275541962092341162602522202993782792835301375 1606938044258990275541962092341162602522202993782792835301376 1606938044258990275541962092341162602522202993782792835301377 1606938044258990275541962092341162602522202993782792835301378 1606938044258990275541962092341162602522202993782792835301379])

In [9]: bi[:] = ak.arange(2**200 - 1, 2**200+4)

In [10]: bi
Out[10]: array([3 4 5 6 7])

@jaketrookman jaketrookman changed the title Closes #2472 Update IndexingMsg to use % instead of mod for setting bigint pdarrays Closes #2472 Update IndexingMsg to use & instead of mod for setting bigint pdarrays Oct 3, 2023
@jaketrookman jaketrookman marked this pull request as ready for review October 4, 2023 18:34
@stress-tess stress-tess changed the title Closes #2472 Update IndexingMsg to use & instead of mod for setting bigint pdarrays Closes #2472, #2800: Update IndexingMsg to use & instead of mod for setting bigint pdarrays Oct 4, 2023
PROTO_tests/tests/indexing_test.py Show resolved Hide resolved
PROTO_tests/tests/indexing_test.py Outdated Show resolved Hide resolved
src/IndexingMsg.chpl Outdated Show resolved Hide resolved
src/IndexingMsg.chpl Outdated Show resolved Hide resolved
PROTO_tests/tests/indexing_test.py Outdated Show resolved Hide resolved
src/IndexingMsg.chpl Outdated Show resolved Hide resolved
src/IndexingMsg.chpl Outdated Show resolved Hide resolved
tests/indexing_test.py Outdated Show resolved Hide resolved
Copy link
Member

@stress-tess stress-tess left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one nit pick on the tests but the logic looks great!

PROTO_tests/tests/indexing_test.py Outdated Show resolved Hide resolved
Copy link
Member

@stress-tess stress-tess left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@stress-tess stress-tess added this pull request to the merge queue Oct 6, 2023
Merged via the queue into Bears-R-Us:master with commit 84fbe90 Oct 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants