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

Add Independent Sparse Thresholds and Sparse Allocation Control Fields #699

Merged
merged 99 commits into from
Nov 30, 2022

Conversation

lroberts36
Copy link
Collaborator

@lroberts36 lroberts36 commented Jul 27, 2022

PR Summary

This PR adds a few different sparse variable features and fixes some bugs:

  • Ability to specify the sparse allocation and deallocation thresholds for individual fields.
  • Functionality to allocate/deallocate sparse fields based on the allocation/deallocation status of another sparse field (i.e. controlled and controlling fields).
  • Allow for default values of unallocated or newly allocated fields to have values other than zero.
  • NaN payload tags for flagging unitialized, newly allocated sparse fields.
  • Sparse advection test that uses these new features.
  • There are also a couple fixes of small bugs in the sparse machinery.
  • Include better checks on whether or not packs containing sparse variables are stale. The old machinery could result in packs being used that were stale.

The first two features are being used downstream in Riot.

PR Checklist

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

@lroberts36 lroberts36 added the enhancement New feature or request label Jul 27, 2022
@lroberts36
Copy link
Collaborator Author

@par-hermes format

@lroberts36
Copy link
Collaborator Author

@pgrete Can you give this a review when you get the chance? Although there might be some more change to the sparse framework as we work on things in Riot, I think at this point it would be good to get this into develop. It had develop had diverged from it significantly over the last month, so I think my original plan of waiting until things were completely stable in Riot may not be a good one (in particular I am thinking of changes coming as we include face, edge, and vertex variables).

Copy link
Collaborator

@pgrete pgrete left a comment

Choose a reason for hiding this comment

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

First bunch of comments, more tomorrow.

Comment on lines +50 to +55
- *Allocation due to neighbors:* If a sparse field on a neighbor block passes ghost data that is
anywhere above the `allocation_threshold`, the receiving block will allocate the sparse field if
it is unallocated before loading the ghost data. `allocation_threshold` is set in the `Metadata`
of a field. A detailed description of sparse ghost zone communication is
given [here](../sparse_boundary_communication.md).
- *Allocation due to other fields:* In some instances, we may want to allocate or deallocate a
Copy link
Collaborator

Choose a reason for hiding this comment

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

It may be good to document the logical relationship between those two mechanism, i.e., are they exclusive or, or, and and, if both are defined if there's an error or which one takes precedence.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think that it should be or between allocation due to neighbors and allocation due to other fields. We should always default to allocating a field if anything requires allocation, since that won't degrade the accuracy of the calculation.

Right now, there is no mechanism for allocation due to other fields in the code.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Right now, there is no mechanism for allocation due to other fields in the code.

Now I'm confused. I thought this PR is about adding this functionality (as described also here in the doc). What am I missing?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry, there are two ideas here. The first is that you would like to always allocate one field whenever some other field is allocated. This is what this PR introduced. The second, which is not implemented, is allocating a field based upon some more complicated criterion in the interior of a block (e.g. the temperature being above/below some threshold).

Copy link
Collaborator

@pgrete pgrete left a comment

Choose a reason for hiding this comment

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

Alright. I'm done with the review. I left a couple of comments (nothing too big) that I'd like to see/hear an answer first before doing the final (swift) approve.

@lroberts36
Copy link
Collaborator Author

Thanks for the review @pgrete! I think I have responded to all your comments.

Copy link
Collaborator

@pgrete pgrete left a comment

Choose a reason for hiding this comment

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

Thanks for addressing my comments.
I'm approving now (but would still be happy for final replies to the unresolved conversations).

Also one final note on sparse in general. The complexity/logic of the implementation is now beyond what I was able to follow based on the docs.
I know there are more pressing concerns in terms of getting things work, but at some (hopefully not too distant future), I'd be very happy to see a more detailed (developer) doc for the internals of the parse machinery.
I'm probably also biased as I again spent hours this week trying to track down #659 with little progress,

@lroberts36 lroberts36 merged commit 249acbb into develop Nov 30, 2022
@Yurlungur Yurlungur deleted the lroberts36/independent-sparse-thresholds branch December 3, 2022 15:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Need per-variable sparse (de)alloc thresholds Sparse "buffer send" threshold is applied to dense variables
3 participants