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

Cmfd refactor to sync with ClosedMOC/3D-MOC #254

Merged
merged 4 commits into from
Feb 3, 2016

Conversation

samuelshaner
Copy link
Contributor

This PR will be accompanied by a PR to ClosedMOC/3D-MOC that syncs up the Cmfd, Vector, and Matrix classes and linalg functions as much as possible. Important changes in this PR include:

  • Shared OpenMP locks: @geogunow suggested that OpenMP locks be shared across objects, where possible, to avoid unnecessary duplication of locks. Cmfd now has an array of locks (by Cmfd cell) that are shared with the Vector and Matrix objects used to solve the diffusion eigenvalue problem.
  • splitEdgeCurrents() corrected: The procedure for splitting edge currents has been fixed to properly handle REFLECTIVE, PERIODIC, and VACUUM boundary conditions.
  • Solver residual initialized to 0.: If left uninitialized, the residual output for iteration 0 is junk (at least for double precision).
  • Vectorized setValues(...) and incrementValues(...) added to Vector.cpp: This implements the Cmfd shared memory optimization that @jtramm started in PR Cmfd shared memory optimization #196.
  • Lattice::getLatticeSurface(...) reverted to general form: The Lattice::getLatticeSurface(...) method made some assumptions on the numbering of the surfaces and edges that is not valid for 3D surfaces, so this method was reverted to the previous general form.
  • splitCorners() -> splitEdgeCurrents(): The splitCorners() routine has been changed to splitEdgeCurrents() and the nomenclature for corners has been changed to edges to reflect the language commonly used elsewhere to describe a cube:

faces-edges-vertices

@wbinventor
Copy link
Member

I'll make a quick review if this after @geogunow does primary thorough review, since I've been using CMFD a great deal. I'm curious, do these.changes affect the results, e.g., the CMFD test results from #251?

@samuelshaner
Copy link
Contributor Author

I just ran a bunch of cases from the sample-inputs, including c5g7-cmfd.py, and the test_cmfd_pwr_assembly and got the exact same results from the code in this PR and the code from PR #251.

@wbinventor
Copy link
Member

Great! Thanks for checking @samuelshaner.


#pragma omp parallel
#pragma omp parallel
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this new spacing correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All other preprocessor directives are indented to the line they address. However, the google c++ style guide says that all preprocessor directives should not be indented. This was accidental, but to be consistent with the style guide I would recommend we make a PR that removes the indent from the directives.

@geogunow
Copy link
Contributor

geogunow commented Feb 3, 2016

I just read through this PR. Everything looks good aside from a couple small spacing comments. I have yet to run this on test cases. In the near future, this will hopefully be done by Travis.

geogunow added a commit that referenced this pull request Feb 3, 2016
Cmfd refactor to sync with ClosedMOC/3D-MOC
@geogunow geogunow merged commit d1b8dd1 into mit-crpg:develop Feb 3, 2016
_cell_locks = new omp_lock_t[num_cells];

/* Loop over all cells to initialize OpenMP locks */
#pragma omp parallel for schedule(guided)
Copy link
Contributor

Choose a reason for hiding this comment

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

Per the new style, fix spacing

@geogunow
Copy link
Contributor

geogunow commented Feb 4, 2016

Oops, forgot I merged this PR

@samuelshaner samuelshaner deleted the cmfd-split-refactor branch February 7, 2016 15:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants