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
Add Material objects to C++ #1045
Conversation
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.
Thanks for this PR @smharper. My only major comment is regarding our use of MATERIAL_VOID
and C_NONE
(see line comment below).
src/material.cpp
Outdated
#include "xml_interface.h" | ||
|
||
//TODO: remove this include | ||
#include <iostream> |
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.
Remove
src/material.cpp
Outdated
Material::Material(pugi::xml_node material_node) | ||
{ | ||
if (check_for_node(material_node, "id")) { | ||
id = stoi(get_node_value(material_node, "id")); |
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 would explicitly write std::stoi
src/material.cpp
Outdated
read_materials(pugi::xml_node* node) | ||
{ | ||
// Loop over XML material elements and populate the array. | ||
for (pugi::xml_node material_node: node->children("material")) { |
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've been using spaces on both sides of :
in a range-for, adopting the style used in the core guidelines.
src/cell.cpp
Outdated
for (pugi::xml_node cell_node: node->children("cell")) { | ||
global_cells.push_back(new Cell(cell_node)); | ||
} | ||
global_cells.shrink_to_fit(); |
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.
If space is reserved for exactly the number of cells there are, why is shrink_to_fit
needed?
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 was just thinking about the wording of the reserve
and shrink_to_fit
requirements in the standards. reserve
could reserve more space than needed, whereas shrink_to_fit
I think is a more strong request for shrinking to the smallest capacity. This is a really small issue though so I'll remove the shrink_to_fit
if you think it's unnecessary clutter.
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.
In practice, reserve(n)
will make the capacity exactly n
if it's greater than the current capacity. I've confirmed with a little program:
#include <iostream>
#include <vector>
int main() {
for (int n = 0; n < 100; ++n) {
std::vector<int> x;
x.reserve(n);
std::cout << "n = " << n << ", capacity = " << x.capacity() << '\n';
}
}
Using g++, icpc, and clang, all give the same results (capacity == n for all n).
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.
So, in your case, shrink_to_fit
will be a no-op; might as well remove it.
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.
Good to know. I'll cut out the unneeded ones.
src/xml_interface.h
Outdated
@@ -17,7 +17,7 @@ check_for_node(pugi::xml_node node, const char *name) | |||
} | |||
|
|||
std::string get_node_value(pugi::xml_node node, const char *name, | |||
bool lowercase=false, bool strip=false); | |||
bool lowercase=true, bool strip=true); |
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 would still prefer these guys to be false by default (i.e., no extra work by default). For any kind of numeric data we are reading from XML, there is generally no reason to lowercase and strip the string. Taking a look at where we are using get_node_value
on the develop branch, most cases actually use the false
default.
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.
Good point, most of our data is numeric. I've switched back to false
.
src/cell.cpp
Outdated
{ | ||
int32_t mat = c->material[i-1]; | ||
if (mat == C_NONE) return F90_NONE; | ||
if (mat == MATERIAL_VOID) return MATERIAL_VOID; |
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.
This is starting to look a little messy. Is there really a need to have a separate value indicating "no material was specified" as opposed to "void material". Maybe we should adopt the following behavior:
<cell>
hasmaterial
attribute but nofill
: single material or distributed materials andCell.fill = -1
<cell>
hasfill
attribute but nofill
: universe/lattice andCell.material = -1
<cell>
has neithermaterial
norfill
: void material with bothCell.material = -1
andCell.fill = -1
.
If we want to check "Is this material filled with a universe/lattice" (for example, geometry_aux.cpp:23
), that would just be Cell.fill >= 0
.
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.
Much cleaner, thanks!
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.
Note, I went with something slightly different than what you've suggested here. Case 3 is prevented (at least when reading from XML) by a fatal_error
. For case 2, Cell.material
is just left as an empty vector.
d6bc6d6
to
f70cd29
Compare
This PR is small in terms of migrating functionality, but it will probably be helpful to do small, fast PRs like these given our pace on the C++ rewrite. This PR adds a basic
Material
class to C++. Right now the only data it owns is theid
, but that's enough for C++ to also take control of the cell-material mapping. This change is probably a pre-requisite for moving the rest of geometry.F90 and plot.F90 over to C++.While I was at it, I also switched most of my old code to our newly-established style for pointers and references.
@paulromano, I also switched your new default arguments to the
get_node_value
function totrue
. That's by far the more common case needed for all the XML inputs I've dealt with. Let me know if you want it to stayfalse
. (Also, I tried to figure out where you neededfalse
in #1042, but I couldn't find it.)