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 all Tie::IxHash w/ Hash::Ordered #21

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@lejeunerenard

lejeunerenard commented Jul 31, 2015

This is virtually a 1-to-1 replacement of Tie::IxHash with Hash::Ordered. Exception noted below.

All tests pass, but found that the Tie::IxHash solution was slightly faster when I did some benchmarks. If you could verify that with your own benchmarks that would be awesome. I don't have much experience benchmarking.

One possible hotspot in my changes is Line #63 in lib/PDL/Factor.pm. I tried using first_index from List::AllUtils to replace the Indices() sub from Tie::IxHash, but couldn't because the $_ inside first_index's block interfered with rmap's $_. Hash::Ordered doesn't have an equivalent for Indices().

Other implementation notes:

  • scalar $hash->keys replaces Tie::IxHash's Length
  • set() replaces Tie::IxHash's Push. Hash::Ordered does have a push() but set() was a closer match.
  • We might want to think of another solution to creating Hash::Ordered::Extension. Creating a new package just to add a way to rename the keys seems a waste of the Hash::Ordered::Extension namespace.

Closes #15

A PR Challenge entry. :)

@zmughal

This comment has been minimized.

Show comment
Hide comment
@zmughal

zmughal Aug 6, 2015

Member

Just wanted to let you know that I haven't forgotten about this! Just need to clear out some time to get this set up for benchmarking.

Member

zmughal commented Aug 6, 2015

Just wanted to let you know that I haven't forgotten about this! Just need to clear out some time to get this set up for benchmarking.

@zmughal zmughal self-assigned this Aug 6, 2015

@zmughal zmughal added the enhancement label Aug 6, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment