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

Ordered history output #1209

Merged
merged 6 commits into from
Nov 14, 2024
Merged

Ordered history output #1209

merged 6 commits into from
Nov 14, 2024

Conversation

brryan
Copy link
Collaborator

@brryan brryan commented Nov 13, 2024

PR Summary

When populating history outputs, we index through a Dictionary of packages, which is an unordered map, and so the order in which we retrieve the packages isn't guaranteed. We've seen this cause inconsistencies in history outputs between platforms, and a more significant concern is that the order of the numbers in a history output could change between simulation runs.

This is a simple fix that just always creates history outputs in alphabetical order of the name of the packages.

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
  • Change is breaking (API, behavior, ...)
    • Change is additionally added to CHANGELOG.md in the breaking section
    • PR is marked as breaking
    • Short summary API changes at the top of the PR (plus optionally with an automated update/fix script)
  • CI has been triggered on Darwin for performance regression tests.
  • Docs build
  • (@lanl.gov employees) Update copyright on changed files

Copy link
Collaborator

@Yurlungur Yurlungur left a comment

Choose a reason for hiding this comment

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

Nice fix. Alternative approach might be to change the map to an ordered one? I think lexicographic key ordering would then just happen for free?

@brryan
Copy link
Collaborator Author

brryan commented Nov 13, 2024

Nice fix. Alternative approach might be to change the map to an ordered one? I think lexicographic key ordering would then just happen for free?

I think that's right, and we could do that too, except we would want to ensure that map ordering of Dictionary doesn't change in the future for the sake of this history output. This issue of history output columns switching midway through a run is potentially disastrous.

In that sense since this PR is probably free from a performance perspective, I'm slightly inclined to just do the sorting here since it's critical that we're sorted at this stage. And even if we have an ordered map later on, this sorting will still just be an extra check.

@Yurlungur
Copy link
Collaborator

Nice fix. Alternative approach might be to change the map to an ordered one? I think lexicographic key ordering would then just happen for free?

I think that's right, and we could do that too, except we would want to ensure that map ordering of Dictionary doesn't change in the future for the sake of this history output. This issue of history output columns switching midway through a run is potentially disastrous.

In that sense since this PR is probably free from a performance perspective, I'm slightly inclined to just do the sorting here since it's critical that we're sorted at this stage. And even if we have an ordered map later on, this sorting will still just be an extra check.

Yeah no objections to doing it this way. I think in the end the difference will be in the weeds.

@adamdempsey90
Copy link
Collaborator

I think it might be useful to document somewhere that the packages are not iterated through in the order that they were added, which one might naively expect.

@Yurlungur
Copy link
Collaborator

Yurlungur commented Nov 13, 2024

I think it might be useful to document somewhere that the packages are not iterated through in the order that they were added, which one might naively expect.

+1

@brryan
Copy link
Collaborator Author

brryan commented Nov 13, 2024

I think it might be useful to document somewhere that the packages are not iterated through in the order that they were added, which one might naively expect.

Good idea, done

@pdmullen pdmullen enabled auto-merge November 13, 2024 23:21
@pdmullen pdmullen merged commit cad5ccd into develop Nov 14, 2024
53 checks passed
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.

4 participants