Conversation
bendudson
left a comment
bendudson
left a comment
There was a problem hiding this comment.
This is really two changes, but they both look sensible.
|
Ok, i will make smaller PRs 👍 |
|
No problem; keep them coming ;) |
|
Please could you add unit tests for the min/max functions? Should be easy enough to copy the existing ones in |
src/field/fieldperp.cxx
Outdated
| 0, nz-1}; | ||
| break; | ||
| default: | ||
| throw BoutException("Field3D::region() : Requested region not implemented"); |
There was a problem hiding this comment.
Exception message should use field perp rather than 3D I think.
| const DataIterator begin() const; | ||
| const DataIterator end() const; | ||
|
|
||
| const IndexRange region(REGION rgn) const; |
There was a problem hiding this comment.
This seems separate from the addition of regions to min/max and doesn't seem to be used, is this in preparation for something else?
There was a problem hiding this comment.
This is unrelated.
It allows to iterate over a fieldperp - but I don't think I use it in the aiolos mesh.
It is rather to unify the fields.
|
Is it worth doing a similar thing for |
|
I am not sure - checkData is not a member function - so not sure that would work. |
|
I think min and max aren't member functions either. |
|
Generally I have tried to reduce the number of member functions: wherever possible code should use the class public API, and not depend on private implementation details. Reducing the size of the class helps make it more comprehensible. Proof by counterexample: the giant mess that is Mesh... |
|
oh, you meant adding a REGION arguments? But I think min, max, checkData and maybe more functions could equally well be defined over |
|
Sorry I should have been clearer! |
|
We could define Field versions, but then would need an efficient way to iterate over them without just casting back to Field2D/Field3D etc. If just the base class then indexing would be a virtual function call. We could perhaps make them template functions, but I don't think that has much advantage over the current scheme of using a consistent interface for the overloaded functions. |
|
@bendudson How do the z boundaries work? |
|
@dschwoerer At the moment Z boundaries are just done in an ad-hoc way e.g. https://github.com/bendudson/isothermal-4field/blob/master/isothermal-4field.cxx#L228 |
| */ | ||
| virtual int getNz() const; | ||
|
|
||
| /// Make region mendatory for all fields |
| * | ||
| * @param[in] f The field to loop over | ||
| * @param[in] allpe Minimum over all processors? | ||
| * @param[in] rgn the boundaries that should be ignored |
There was a problem hiding this comment.
I think this should be "rgn Calculate the minimum inside this region".
Currently it sounds like rgn should contain the points you want to ignore, rather than the points you want to consider
There was a problem hiding this comment.
rgn can only be used to ignore the boundaries - no general region.
maybe
rgn The boundaries to be included in the calculation
?
There was a problem hiding this comment.
What about RGN_ALL? We're also moving towards supporting very general regions which may even be some subset of the interior points, so this should be
* @param[in] rgn The region to calculate the minimum over
and likewise for max
There was a problem hiding this comment.
RGN_ALL means to include all boundaries - so matches the description as far as I understand.
I wrote the documentation for what is currently done - not what will be possible in the future.
I can change of course - but I am not the expert on the generalized region thing, am not sure whether or when it is going to get merged, when that function is adapted, so I didn't match it.
4 participants
add the region to the interface of the min/max functions.
Part of #742
Also add dataiterator interface to fieldperp