-
Notifications
You must be signed in to change notification settings - Fork 456
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
Hex lattice #313
Hex lattice #313
Conversation
This build is currently causing a weird error in read_xml_files_materials_t
Conflicts: src/input_xml.F90
Thanks to @nhorelik for the vast majority of changes in this commit.
Intel Fortran raises a compilation error when a pointer to a polymorphic object is passed from inside a 'type is' block to a dummy argument which is a pointer to the object's parent class.
Checked all non-CMFD tests; they all pass except score_flux_yn; need to investigate that failure closely
Conflicts: src/initialize.F90
Thanks for this @smharper! This is a biggie, so I'm going to say we should shoot for inclusion in v0.7. I'll start reviewing/testing once all the v0.6.1 stuff is complete. |
Commit f6b7f5f accidentally removed the loc = minloc... line
Conflicts: src/geometry.F90 src/geometry_header.F90 src/initialize.F90
The bug that I mentioned in the comment above was fixed in #359 so I've now removed the crappy fix implemented in 11c490b (it turned out to cause another bug anyway). I have also moved the stuff in the update_lattices.py script over to the update_inputs.py script. At this point, I don't know of any standing issues with this PR. |
.. math:: | ||
:label: rect_indexing | ||
|
||
i_x = \Bigg \lceil \frac{x - x_0}{p_0} \Bigg \rceil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did \left and \right (instead of \Bigg) not work for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like \left and \right work fine. I've switched over to those now.
Need to update models in examples/ folder. |
p % coord % next% lattice = c % fill | ||
p % coord % next% lattice_x = i_xyz(1) | ||
p % coord % next% lattice_y = i_xyz(2) | ||
p % coord % next% lattice_z = i_xyz(3) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very clean, I love it!
if (check_for_node(node_lat, "width")) then | ||
call warning("The use of 'width' is deprecated and will be disallowed & | ||
&in a future release. Use 'pitch' instead. The utility openmc/& | ||
&src/utils/update_lattices.py can be used to automatically update & |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
update_inputs.py
integer :: line_wrap ! length of line | ||
integer :: length ! length of message | ||
integer :: indent ! length of indentation | ||
character(:), allocatable :: message ! input message with a prefix |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, allocatable characters break building with gfortran 4.6. Normally, I wouldn't care much, but since we're moving to Travis CI for testing and their VMs still use Ubuntu 12.04 (which in turn uses gcc 4.6), this would prevent building there. Would you mind re-coding this to remove allocatable characters?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think reworking warning
is worth the effort right now. If it's okay with you, I'd rather just revert to the develop version of warning
.
Hexagonal lattice capability
Thanks again @smharper! |
<hex_lattice id="10" n_rings="3" outer="1"> | ||
<center> 0.0 0.0 </center> | ||
<pitch> 1.0 </pitch> | ||
<universes> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this PR has been posted, but is it a typo that this example hex lattice specification does not include the "n_rings" element? Based on the description of "n_rings" above, I believe this is a required element.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "n_rings" is present as an attribute in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I totally missed that. Sorry for the confusion!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries
This pull request gives OpenMC the ability to simulate geometries with hexagonal lattices. In addition to adding the new hexagonal lattice features, this PR makes significant changes to the existing code. Here are the top-level changes:
type
element/attribute.pitch
is now used rather thanwidth
when specifying lattices in the geometry.xml file.type
andwidth
, I added deprecation warnings to input.F90. I know that we don't technically need them since we are still in 0.x.y releases, but I think it's more polite to our users. I also added a Python utility to automatically update geometry.xml files.Low-level changes:
get_lat_trans
,get_lat_indices
, andis_valid_lat_index
.Lattice
class.Lattice
objects can be passed around by most functions without worrying about whether they are actually instances of theRectLattice
orHexLattice
types. Unfortunately, it also means that the globallattices
array needs to be accessed likelattices(i) % obj
rather than justlattices(i)
. Sometime in the future I would like to convert the lattice primitives likeget_lat_trans
into type-bound procedures.lattice_edge
calculations fromfind_cell
. This had no effect on the test suite, and I have not seen any damage caused by it's removal in my larger test calculations.distance_to_boundary
stores lattice distances and surface distances in different variables in order to address issue Lattice Corner Crossing #258Design decisions that may affect performance or readability:
distance_to_boundary
uses a lot of expensive calculations to measure distances relative to the neighbor cells rather than the current cell. This algorithm is also used due to it's superior finite precision performance.distance_to_boundary
for negative lattice distances. I needed this for debugging, but I am considering leaving it in so that no future developers accidentally make changes that allow negative distances.cross_lattice
now usesget_lat_trans
which is a little bit more expensive than the older method of subtracting cell widths from the particle position, but it's agnostic to the lattice type and provides better finite precision performance (I had a lot of finite precision issues, can you tell?).Verification and validation:
I can provide my V&V models to anyone who wants them. I'll also try to rerun them after we've gotten through a lot of the review process to make sure no errors have sneaked in. Also, I should mention that none of the regression test results are different (despite the changes to the handling of rectangular lattices) with the exception of score_flux_yn. I haven't updated that test yet because I am investigating it to see if I can find the reason for the change.