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

Rename CellVariable to Variable, etc. #867

Merged
merged 15 commits into from
May 26, 2023
Merged

Conversation

lroberts36
Copy link
Collaborator

@lroberts36 lroberts36 commented Apr 28, 2023

PR Summary

I am going back to start on adding face, etc. variables. PR #764 has gotten substantially out of sync with develop, so I think it is easier at this point to just redo what is there. I plan to do what is in that PR over a few new, smaller PRs.

This PR takes the first, simplest step and just removes Cell and cc from the names of things in most places. It also adds checks in the communication routines that variables with Metadata::WithFluxes and Metadata::FillGhosts set also have Metadata::Cell set.

I renamed the namespace cell_centered_bvars to var_boundary_comm and changed the directory bvals/cc to bvals/bnd_flx_communication so this is a breaking change for downstream codes.

There is nothing substantive here I think, but any comments on the naming choices I made are welcome.

I didn't rename MeshBlock::pvar_cc, MeshBlock::vars_cc_ and MeshRefinement::pvars_cc_, or any of the cc that show up in amr_loadbalance.cpp since these currently should only contain cell centered variables.

PR Checklist

  • Code passes cpplint
  • 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

@bprather
Copy link
Collaborator

I'm happy with this. Planning on pulling this into KHARMA before I put in a review but could pass it faster if we want to expedite things.

@lroberts36
Copy link
Collaborator Author

@bprather: I think it would be helpful to make sure there isn't some issue with the renaming when it is pulled into KHARMA (and other downstream codes). There is no need for a quick review, I was just going to put up other PRs that depend on this one and I doubt they will go as quickly since this was essentially a search and replace.

@lroberts36 lroberts36 mentioned this pull request Apr 28, 2023
12 tasks
@lroberts36 lroberts36 mentioned this pull request May 1, 2023
13 tasks
Copy link
Collaborator

@brryan brryan left a comment

Choose a reason for hiding this comment

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

Looks good to me, I'm going to approve this now.

That said I did raise some minor questions about naming. Feel free to ignore but since this is a breaking change now is probably the time to optimize and future-proof (mainly for swarms) these names if such things are necessary.

example/stochastic_subgrid/stochastic_subgrid_driver.cpp Outdated Show resolved Hide resolved
example/stochastic_subgrid/stochastic_subgrid_driver.cpp Outdated Show resolved Hide resolved
src/bvals/boundary_conditions.cpp Outdated Show resolved Hide resolved
@lroberts36 lroberts36 added enhancement New feature or request breaks-downstream labels May 2, 2023
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.

I'm excited to see this finally moving!

example/stochastic_subgrid/stochastic_subgrid_driver.cpp Outdated Show resolved Hide resolved
Copy link
Collaborator

@forrestglines forrestglines left a comment

Choose a reason for hiding this comment

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

I tested the rename in AthenaPK, works fine with a simple find-and-replace in one file. The full test suite is still running but it's passing so far.

Edge and face variable will all live together with cell variables under Variable with metadata to distinguish them, right?

On the rename of bvals_cc_in_one.hpp to bvals_in_one.hpp, do you foresee all variable boundary communications living under that header?

example/stochastic_subgrid/stochastic_subgrid_driver.cpp Outdated Show resolved Hide resolved
@lroberts36
Copy link
Collaborator Author

lroberts36 commented May 3, 2023

I tested the rename in AthenaPK, works fine with a simple find-and-replace in one file. The full test suite is still running but it's passing so far.

Thanks for testing it out

Edge and face variable will all live together with cell variables under Variable with metadata to distinguish them, right?

Yeah, that is what is implemented in #868.

On the rename of bvals_cc_in_one.hpp to bvals_in_one.hpp, do you foresee all variable boundary communications living under that header?

Yeah, for boundary communication variables of all types will be communicated when *BoundBufs are called (see #872).

Copy link
Collaborator

@bprather bprather left a comment

Choose a reason for hiding this comment

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

Using this successfully in KHARMA for a week now. I don't have boundary communication working downstream, but all the naming was easy to update & I'm happy with the names, and the code works just fine.

@pgrete pgrete enabled auto-merge (squash) May 26, 2023 12:31
@pgrete pgrete merged commit 4304bed into develop May 26, 2023
@BenWibking BenWibking mentioned this pull request Jun 27, 2023
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaks-downstream enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants