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

map instead of unordered_map for sparse pool #894

Merged
merged 4 commits into from
Jun 21, 2023

Conversation

Yurlungur
Copy link
Collaborator

@Yurlungur Yurlungur commented Jun 20, 2023

PR Summary

The sparse pool uses an unordered map. However, sparse ids have an implicit ordering, they are integers after all. While not a bug exactly, the unordered map can mangle the ordering and can produce un-intuitive behavior where a user may request sparse IDs and expect the pool to return an ordered list. This PR changes the map to an ordered one (i.e., a heap) so traversal through it always produces sparse IDs sorted from smallest to largest.

PR Checklist

  • Code passes cpplint
  • New features are documented.
  • Adds a test for any bugs fixed. Adds tests for new features.
  • Code is formatted
  • Changes are summarized in CHANGELOG.md
  • CI has been triggered on Darwin for performance regression tests.
  • Docs build
  • (@lanl.gov employees) Update copyright on changed files

@Yurlungur Yurlungur added bug Something isn't working breaks-downstream labels Jun 20, 2023
@Yurlungur Yurlungur self-assigned this Jun 20, 2023
Copy link
Collaborator

@pdmullen pdmullen left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@lroberts36 lroberts36 left a comment

Choose a reason for hiding this comment

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

LGTM

@Yurlungur
Copy link
Collaborator Author

Cuda CI test seems to be failing here, but it's working when I run on a local cuda machine. @lroberts36 is reporting similar issues. @pgrete is there perhaps something wrong with the CI machine?

@Yurlungur Yurlungur enabled auto-merge June 21, 2023 19:29
@Yurlungur Yurlungur merged commit 099d20e into develop Jun 21, 2023
@pgrete
Copy link
Collaborator

pgrete commented Jun 22, 2023

Cuda CI test seems to be failing here, but it's working when I run on a local cuda machine. @lroberts36 is reporting similar issues. @pgrete is there perhaps something wrong with the CI machine?

Cuda tests did eventually pass, didn't they? And yes, as I mention on Matrix yesterday the machine is more heavily used. now so it's likely just a fight for resources.
Given that it's not "my" node, there's currently very little I can do to fix this.

@Yurlungur Yurlungur deleted the jmm/sparse-id-ordering branch June 22, 2023 16:26
@Yurlungur
Copy link
Collaborator Author

Cuda tests did eventually pass, didn't they?

Yes eventually. Perhaps just needs to be timed for nighttime in the EU

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaks-downstream bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants