Add VolumeHistogram vector postprocessor #7803
Conversation
dschwen
force-pushed
the
volumehistogram_7801
branch
from
4586db9
to
7cf31c6
Compare
| virtual void initialize(); | ||
| virtual void execute(); | ||
| virtual void finalize(); | ||
| virtual void threadJoin(const UserObject & y); |
There was a problem hiding this comment.
It's interesting that this new test failed. Before diving in, make sure these are all getting called properly (i.e. declare them override)
There was a problem hiding this comment.
Ah, right, general vs. element vector postprocessors is still a bit muddy to me. Technically those are just user objects, right, so they should work as expected. I'll investigate.
There was a problem hiding this comment.
Yes, element types should have threadJoin defined and called on them.
| _max_value(getParam<Real>("max_value")), | ||
| _deltaV((_max_value - _min_value) / _nbins), | ||
| _value(coupledValue("variable")), | ||
| _bin_center(declareVector(getVar("variable", 0)->name())), |
There was a problem hiding this comment.
Interesting that you've chosen to make the name dynamic. That's pretty cool for seeing different column names in your output file. The downside of this is that it makes it a little trickier to couple to this vector from another object should you need that. Are you sure you want to do it this way?
There was a problem hiding this comment.
Well, I don't really have a strong opinion on it. I thought it was nice to have the data file self-document.
There was a problem hiding this comment.
It is, it's just that if you plan to couple to this vector having the name change out from underneath you will be a bit of a pain. If that's not your use case, no problem. Just a suggestion.
There was a problem hiding this comment.
Yeah, let's keep it as it is. The same pattern is uses in SamplerBase and similarly in VectorOfPostprocessors.
| VolumeHistogram::initialize() | ||
| { | ||
| // reset the histogram | ||
| _volume.assign(_nbins, 0.0); |
| void | ||
| VolumeHistogram::finalize() | ||
| { | ||
| gatherSum(_volume); |
There was a problem hiding this comment.
but.. but.. you not only allow it, you provide it. It is a freaking MOOSE function!!!!
There was a problem hiding this comment.
I'm just complaining. I plan to deprecate it moving forward.
There was a problem hiding this comment.
To be clear, I'm not asking you to change this on this PR.
To answer your question:
_communicator.sum()
I don't see any purpose to wrapping a very, very small subset of the methods available on the communicator. It doesn't make anything "easier" or clearer. I'd rather see that the communicator object is involved and just go look at the available methods. Also if you need to do a sum and then some other operation that's not wrapped in the same post processor should you mix these styles?
There was a problem hiding this comment.
See my thoughts on these convenience functions here: #7820
| const VolumeHistogram & uo = static_cast<const VolumeHistogram &>(y); | ||
| mooseAssert(uo._volume.size() == _volume.size(), "Inconsistent volume vector lengths across threads."); | ||
|
|
||
| for (unsigned int i = 0; i < _volume.size(); ++i) |
There was a problem hiding this comment.
In fact this could just be done with a range based for loop
There was a problem hiding this comment.
Sorry, missed that, scratch the range based suggestion. I don't think std::distance should be used for things like this.
dschwen
force-pushed
the
volumehistogram_7801
branch
from
7cf31c6
to
c9635c9
Compare
dschwen
force-pushed
the
volumehistogram_7801
branch
from
c9635c9
to
513c5df
Compare
| VectorPostprocessorValue _volume_tmp; | ||
|
|
||
| /// aggregated global volume vector | ||
| VectorPostprocessorValue & _volume; |
There was a problem hiding this comment.
@friedmud - Look at the implementation here:
This is basically the correct way to implement a VectorPostprocessor given the current system constraints. We have to (or probably should) create one or more VectorPostprocessorValue members to use for processing information during execute() and/or threadJoin(), then we have the final member that we hold a reference to from the call to declareVector().
See the rest of my comments and a suggestion for an alternative design on #7809.
Let's the user obtain a histogram of the volume fractions occupied by a certain variable value.
Closes #7801