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

Draft: AMR preliminaries #79

Closed
wants to merge 2 commits into from
Closed

Conversation

hughcars
Copy link
Collaborator

@hughcars hughcars commented Aug 1, 2023

Description of changes:
Minor changes in advance of the AMR PRs:

  • Some constexpr
  • Fix some enum and unused variable warnings
  • Broke out the default integration rules into a function, this was required by an initial implementation, but no longer necessary. Included however as it was a nice simplification from multiple inheritance.
  • Refactor the multgrid helper function signatures to use overloads, and allow building p-multigrid or single mesh/fec "multigrids".
  • Added GlobalTrueVSize() to main operator classes, necessary for future estimation PR.

Issue #, if available:
#2

@hughcars hughcars added the minor A minor issue or improvement label Aug 1, 2023
@hughcars hughcars force-pushed the hughcars/amr-preliminary-reorg branch from 52147a2 to ef96f4b Compare August 1, 2023 16:24
@sebastiangrimberg sebastiangrimberg force-pushed the sjg/driven-port-debug branch 4 times, most recently from 9a51e9f to 393b428 Compare August 8, 2023 18:16
@hughcars hughcars force-pushed the hughcars/amr-preliminary-reorg branch 4 times, most recently from b1ef164 to 31af414 Compare August 15, 2023 15:38
@hughcars hughcars changed the base branch from sjg/driven-port-debug to sjg/libceed-pa-dev August 15, 2023 15:45
@hughcars
Copy link
Collaborator Author

Changed the base to sjg/libceed-pa-dev in anticipation of merges.

@sebastiangrimberg
Copy link
Contributor

Changed the base to sjg/libceed-pa-dev in anticipation of merges.

To me the changes to integrator.hpp, multigrid.hpp, and main.cpp all seem unnecessary and don't really provide any added value. At that point you might as well just fold this into your first AMR PR, unless it makes sense to keep the meshio.cpp constexpr changes separate in which case that's fine too to merge in ASAP.

@hughcars
Copy link
Collaborator Author

Changed the base to sjg/libceed-pa-dev in anticipation of merges.

To me the changes to integrator.hpp, multigrid.hpp, and main.cpp all seem unnecessary and don't really provide any added value. At that point you might as well just fold this into your first AMR PR, unless it makes sense to keep the meshio.cpp constexpr changes separate in which case that's fine too to merge in ASAP.

These were mainly the small style type changes I did whilst doing exploratory work within the amr-dev branch, but they weren't really related to the actual prs coming. The approach we've had in previous PRs is to aim for smallness and to avoid any extras, so I broke them out of the main PR sequence in anticipation.

The only thing that's truly needed from here is that NDof, which I will change to GlobalTrueVSize, the rest are unrelated to the ultimate PR sequence but I thought were slight improvements:

  • The constexpr is more idiomatic c++.
  • The immediately invoked lambda in main.cpp allows for binding a const variable.
  • Namespace over static method is also more idiomatic c++. Moving the quad rule into a function was helpful in an early iteration of the estimation algorithm, where I had been making a function that built the element integral operator on the fly. I figured it expands the capability of getting the rules without needing inheritance. Basically, that it can be done with namespace rather than a static method means it ought to be.
  • The multigrid constructor methods makes building without boundary conditions more obvious (at least to me) compared to passing the nullptr. This makes the call sites in the estimation class look neater too, with the dispatch being moved to compile time, rather than on the runtime status of the pointers within the body. The templated template also has the nice property it plays nice with passing in a unique_ptr or with passing a vector of unique ptrs, which is nice if we're getting rid of the grid hierarchy as h multigrid goes away (I had a version of the main helpers that only did P-multigrid for instance, and used the essential boundary function). I considered using default arguments of nullptr, but that doesn't help if dim then needs to be specified.

Base automatically changed from sjg/libceed-pa-dev to main August 22, 2023 17:14
@hughcars hughcars force-pushed the hughcars/amr-preliminary-reorg branch 2 times, most recently from 0a16a16 to 7d839db Compare August 23, 2023 18:33
sebastiangrimberg added a commit that referenced this pull request Aug 26, 2023
sebastiangrimberg added a commit that referenced this pull request Aug 29, 2023
sebastiangrimberg added a commit that referenced this pull request Aug 29, 2023
sebastiangrimberg added a commit that referenced this pull request Aug 30, 2023
sebastiangrimberg added a commit that referenced this pull request Aug 30, 2023
@hughcars hughcars force-pushed the hughcars/amr-preliminary-reorg branch 2 times, most recently from 036d43e to 76ef2b1 Compare September 1, 2023 14:22
- Some of these are necessary in the AMR branch
- Some of these are minor cleanup/rationalization
- Some are aesthetics
@hughcars hughcars force-pushed the hughcars/amr-preliminary-reorg branch from 76ef2b1 to 090b3b8 Compare October 2, 2023 16:16
@hughcars
Copy link
Collaborator Author

hughcars commented Oct 3, 2023

Being rolled into other PRs.

@hughcars hughcars closed this Oct 3, 2023
@sebastiangrimberg sebastiangrimberg deleted the hughcars/amr-preliminary-reorg branch October 3, 2023 20:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor A minor issue or improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants