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

Cleanup element map and decrease size of ChildSize/MortarSize enums #6032

Merged
merged 6 commits into from
May 30, 2024

Conversation

nilsdeppe
Copy link
Member

Proposed changes

  • goal is to further reduce memory overhead
  • removes what I can from ElementMap without breaking anything
  • Reduces the size of ChildSize/MortarSize but does not attempt to optimize any use cases specifically.
  • The big thing that would help a lot is removing the block maps from the element map and instead just grabbing the block map from the global cache. I think the elliptic solver uses the block map in a non-trivial way, right @nilsvu ? It might make more sense to just get rid of ElementMap all together in the evolution code and just have the compute tags handle the calls direction.

Upgrade instructions

Code review checklist

  • The code is documented and the documentation renders correctly. Run
    make doc to generate the documentation locally into BUILD_DIR/docs/html.
    Then open index.html.
  • The code follows the stylistic and code quality guidelines listed in the
    code review guide.
  • The PR lists upgrade instructions and is labeled bugfix or
    new feature if appropriate.

Further comments

@nilsdeppe nilsdeppe requested a review from kidder May 28, 2024 16:00
@nilsvu
Copy link
Member

nilsvu commented May 28, 2024

Yes I think we can get rid of ElementMap, see the notes I wrote in this file: https://github.com/sxs-collaboration/spectre/blob/develop/src/Domain/Python/ElementMap.cpp. I think we can just construct a Composition coordinate map instead of the ElementMap where it's needed. The function to create a Composition element map is already implemented in the file I linked. We could factor that out to a free function.

@nilsdeppe
Copy link
Member Author

Right, though if I'm reading the comment right that's saying replace ElementMap by a composition? The goal is to not store the block maps in the element databoxes, which means doing a composition is an issue

@nilsvu
Copy link
Member

nilsvu commented May 28, 2024

No need to store the (composition) element map in the element databoxes either in most cases. It can always be constructed on the fly just from the ElementID and the domain/block.

Note: I remember vaguely that some coordinate maps have caches (ylm interpolation info or something like that), so removing them from the element databox may have an effect. Can't find the code right now though, so maybe this was just an idea. I only find src/Domain/CoordinateMaps/TimeDependent/Shape.cpp where the interpolation info doesn't seem to be cached.

@nilsdeppe
Copy link
Member Author

Oh, I see. Sure! What Larry and I had discussed was possibly having the coordinates/Jacobians compute tags handle the compositions in some manner as needed. But I have no idea what all this looks like exactly in the elliptic code. I think in the evolution code we could just store a pointer in ElementMap that points to the global cache, or just have the compute items.

Hmm, I don't remember us caching anything. SpEC does that a lot, but I thoguht we intentionally avoided it because of memory overhead. But there's too much code now, so I don't honestly remember :)

@knelli2
Copy link
Contributor

knelli2 commented May 29, 2024

Just an FYI, I'm planning a lot of interpolation framework changes around using compute tags to have coordinates/tensors in different frames

@kidder kidder merged commit 26034ef into sxs-collaboration:develop May 30, 2024
22 checks passed
@nilsdeppe nilsdeppe deleted the cleanup_element_map branch August 6, 2024 21:54
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