add clustering user obejcts #1201
Conversation
adding eeid_adder
doc/content/source/userobjects/ValueRangeHeuristicUserObject.md
Outdated
Show resolved
Hide resolved
doc/content/source/userobjects/ValueRangeHeuristicUserObject.md
Outdated
Show resolved
Hide resolved
| MooseVariableBase & _metric_variable; | ||
|
|
||
| /// AuxiliarySystem reference | ||
| AuxiliarySystem & _auxiliary_system; |
There was a problem hiding this comment.
can this be made const? MOOSE style is to make as many things const as possible. Are we modifying the auxiliary system in this class, or any derived classes?
There was a problem hiding this comment.
We do serialize the solution. I think that prevents me to make the AuxiliarySystem const.
| _extra_element_id_names( | ||
| getParam<std::vector<ExtraElementIDName>>("extra_element_integer_names")), | ||
| _eeiid_values(isParamValid("values") ? getParam<std::vector<int>>("values") | ||
| : std::vector<int>(_extra_element_id_names.size(), -1)) |
There was a problem hiding this comment.
should -1 be NOT_VISITED?
There was a problem hiding this comment.
NOT_VISITED comes from the clustering context. In ParsedElementIDMeshGenerator, -1 is just a default value used when users don't provide a preferred value for an eeid they want in the mesh. We do provide a description of it in line 21 as well.
I can declare a variable _default = -1 and refer it in line 37.
Co-authored-by: April Novak <novak@berkeley.edu>
| type = Exodiff | ||
| input = example.i | ||
| exodiff = example_out.e | ||
| requirement = "cluster_id_aux needs to be the same" |
There was a problem hiding this comment.
All of the test requirement fields should be complete sentences which should be understandable to someone with only a surface-level understanding of the code. If you look around the existing tests, they all begin with The system shall ....
For instance, I am not clear what this requirement is testing per se. Since it seems to be the only test in this directory, is it just testing correctness of the value range heuristic? If so, a better requirement would be The system shall be able to cluster elements according to their values being within a given range. (I am not sure if this makes sense for this test, because I can't tell from just the requirement what this is meant to cover)
There was a problem hiding this comment.
This same comment applies to all the test requirements, but I'll just write it here for conciseness
There was a problem hiding this comment.
Furthermore, the test requirements should be unique across tests. I see that a few other of the tests in this PR use identical requirement tags, perhaps because a tests file was copied and this parameter was overlooked in updating?
|
Job Precheck, step Clang format on 0eb895a wanted to post the following: Your code requires style changes. A patch was auto generated and copied here
Alternatively, with your repository up to date and in the top level of your repository:
|
5 participants
closes #1200 and #1127