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

[Trivial] Reorganize LogicalLocation code #1028

Merged
merged 16 commits into from
Apr 11, 2024
Merged

Conversation

lroberts36
Copy link
Collaborator

@lroberts36 lroberts36 commented Mar 20, 2024

PR Summary

Just move some files around and split up some files to make the code easier to navigate. Also removes a little bit of dead code.

PR Checklist

  • Code passes cpplint
  • Code is formatted
  • Changes are summarized in CHANGELOG.md
  • (@lanl.gov employees) Update copyright on changed files
  • Forest Of Octrees #1009 is merged

@lroberts36 lroberts36 changed the base branch from develop to lroberts36/add-forest-of-octrees March 20, 2024 22:04
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.

Good reorg 👍

@pgrete
Copy link
Collaborator

pgrete commented Mar 27, 2024

Are you planning to merge this to the forrest branch or later (separately) into develop once forrest is merged?

@lroberts36
Copy link
Collaborator Author

@pgrete: I was thinking later after the forest branch is merged. This would just make the diff even bigger I think.

@lroberts36 lroberts36 mentioned this pull request Mar 27, 2024
7 tasks
Comment on lines -147 to -149
void SetNeighbor(LogicalLocation inloc, int irank, int ilevel, int igid, int ilid,
int iox1, int iox2, int iox3, NeighborConnect itype, int ibid,
int itargetid, int ifi1 = 0, int ifi2 = 0);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is no longer used anywhere (constructors are used instead), so I removed it.

Comment on lines +21 to +22
#ifndef MESH_FOREST_BLOCK_OWNERSHIP_HPP_
#define MESH_FOREST_BLOCK_OWNERSHIP_HPP_
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This previously sat in logical_location.*

@@ -53,31 +53,6 @@ bool LogicalLocation::Contains(const LogicalLocation &containee) const {
return (shifted_lx1 == lx1()) && (shifted_lx2 == lx2()) && (shifted_lx3 == lx3());
}

std::array<int, 3> LogicalLocation::GetOffset(const LogicalLocation &neighbor,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

unused

Comment on lines -361 to -363
// TODO(LFR): Remove this
block_ownership_t
DetermineOwnership(const LogicalLocation &main_block,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

just moved for now, next forest PR removes this though.

@lroberts36 lroberts36 changed the base branch from lroberts36/add-forest-of-octrees to develop April 2, 2024 20:08
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.

LGTM I think this reorg makes sense.

@pgrete pgrete enabled auto-merge (squash) April 11, 2024 08:24
@pgrete pgrete merged commit 2d177f2 into develop Apr 11, 2024
49 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.

3 participants