Add multiple parallel slices #1345
Conversation
| @@ -226,30 +228,42 @@ class Field3D : public Field, public FieldData { | |||
|
|
|||
| /// Check if this field has yup and ydown fields | |||
| bool hasYupYdown() const { | |||
| return (yup_field != nullptr) && (ydown_field != nullptr); | |||
| return !yup_fields.empty() and !ydown_fields.empty(); | |||
There was a problem hiding this comment.
If we added a bool hasValidYUpDown, could be return hasValidYUpDown, since if mergeYupYdown() has been called, then calling yup(), etc., should be allowed so this should return true even if yup_fields is empty.
I'd been deleting yup/ydown fields on #1176, but now I've changed that to calling setHasValidYUpDown(false). 4a7cbcf is meant to have all the places that yup/ydown should be marked as invalid.
There was a problem hiding this comment.
I think it might just be cleaner to delete the parallel slices where we need to invalidate them. I'm not sure there's anything to be gained from keeping them
There was a problem hiding this comment.
👍 am I right in thinking that the std::vector will not deallocate when we call clear, so we're not actually losing anything by deleting them?
There was a problem hiding this comment.
std::vector::clear erases its contents, so the values will be lost but it does retain its capacity though, so next time we create the parallel slices we won't need to allocate any new memory, if that's what you meant.
This changes the behaviour of mergeYupYdown: afterwards, the parallel slices are not valid
* next: (93 commits)
Fix use of abs in fieldgenerators
Fix use of abs in utils.hxx
Fix use of abs in cyclic_reduction
Fix use of abs in imex-bdf2 solver
Fix abs use in slepc solver
Fix use of abs in rkgeneric
Improve some comments; use uniform initialisation more consistently
Use `Options::force` to overwrite options quietly
Default initialise field sizes and Field::bndry* status
Default initialise boundaryIs{Copy,Set} in FieldData
Default initialise some Field3D/2D members
Add move constructor for Field2D
Add swap function for Field2D
Add move constructor for Field3D
Make test-command-args quieter
Add test for clobbering datadir from commandline
Fix:Escaping in makefile
Documentation improvements
Improve documentation
Make sure command line short-options override options file settings
...
|
I think this is the right direction to go in, but unfortunately, it changes both the semantics and behaviour in a backwards-incompatible fashion.
would no longer hold. Is this acceptable? This PR needs to make the non-trivial parallel transforms actually calculate multiple parallel slices for it to be useful. I'm working on writing some sanity/unit tests for the shifted metric so that I can change that happily. |
|
👍 I think the backwards-incompatibility is acceptable; anyone who has physics-model code that would notice the change is surely following |
* next: (347 commits) Parameterise FFT tests over both even- and odd-length real signals Fix bug in `irfft(Array)`: length of original signal is needed Add Array::resize to simplify changing the size of an Array Use type alias instead of typedef Move ShiftedMetric::cmplx/cmplxLoc into shiftZ; make shiftZ const Fix shiftedmetric test fixture Move some initial setup into ShiftedMetric test fixture Add more ShiftedMetric tests Add region argument for test field-field comparisons Basic test for shiftedmetric Add test helpers for filling Fields with values Move map and function definitions into source file for bout_types Support loc argument to Laplacian::tridagCoefs Fix elm-pb MMS test boundary conditions Remove deprecated invert_laplace functions from tests Deleting unused example code Almost all invert_laplace calls removed Removing more invert_laplace uses Make absence of fftw-wisdom non-fatal in configure Use unique_ptr for LaplaceXZcyclic::cr ...
Forward-FFT whole field, then inverse-FFT for each parallel slice. Will make it easier to generalise to multiple parallel slices
Tighten up the unit tests to catch this
|
I think this is about ready to go in. I've not added support for multiple parallel slices to FCI yet. Requires some thinking about. I'm also pondering deprecating the current // Call mesh->getParallelTransform().calcYupYdown()
void createParallelSlices();
// Delete parallel slices; replace mergeYupYdown
void clearParallelSlices();
// Replace hasYupYdown
bool hasParallelSlices() const;
// Replace yup/ydown/ynext
Field3D& getParallelSlice(int dir); |
|
I think the suggested change is a good idea. It makes things a bit more
logical and flexible.
I'm absolutely no expert but I can imagine that this could be slightly
slower (to call getparallelslice 2-4 times per derivative function).
…On Tue, 15 Jan 2019, 17:07 Peter Hill ***@***.*** wrote:
I think this is about ready to go in. I've not added support for multiple
parallel slices to FCI yet. Requires some thinking about.
I'm also pondering deprecating the current yup/ydown, etc. methods and
replacing them with:
// Call mesh->getParallelTransform().calcYupYdown()void createParallelSlices();// Delete parallel slices; replace mergeYupYdownvoid clearParallelSlices();// Replace hasYupYdownbool hasParallelSlices() const;// Replace yup/ydown/ynext
Field3D& getParallelSlice(int dir);
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#1345 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AFQ8TsC-Wleu6Hb3zcOogTxbijb6Qo1iks5vDfzOgaJpZM4X_wBq>
.
|
@bshanahan it's OK: if you set |
| @@ -207,14 +207,13 @@ template <typename T> | |||
| T DDY(const T& f, CELL_LOC outloc = CELL_DEFAULT, const std::string& method = "DEFAULT", | |||
| REGION region = RGN_NOBNDRY) { | |||
| AUTO_TRACE(); | |||
| if (std::is_base_of<Field3D, T>::value && f.hasYupYdown() | |||
| && ((&f.yup() != &f) || (&f.ydown() != &f))) { | |||
| if (std::is_base_of<Field3D, T>::value && f.hasYupYdown()) { | |||
There was a problem hiding this comment.
This hasn't changed, but this condition means that Field2D always goes through the "aligned" branch. It should actually equally be able to go through the orthogonal branch without any issue now (the previous case was that f.yup() != &f would always be false for Field2D so it would always go through the second branch anyway, so the Field3D check was just a compile-time way of ensuring this). I'm not sure which is preferred/more efficient etc. but might be worth experimenting with.
Secondly I think you proposed providing alternative names rather than yup/ydown -- is it worth using the new name in the function here (i.e. f.hasParallelSlices() rather than f.hasYupYdown()), or have these renames not happened yet?
There was a problem hiding this comment.
It would be nice to be able to treat them the same!
Renames haven't happened, I was thinking of doing them in another PR in case there was any discussion on the names, but happy to just do them here (names can always be changed before the next release)
|
I think this is about done finally. I'll make separate PRs for introducing nicer synonyms for yupydown stuff and further tidying up of the parallel transforms. |
* next: (99 commits) Fix typo in documentation Silence warnings in MMS/time Remove some commented out bits from MMS/time Test fix: don't use adaptive time stepping when measure time scaling Further generalise derivatives unit test to fourth order methods Fix comments in derivatives test Be more generous with allowed order to count as pass Add line to trace manipulation section Update unit tests for assignment from non-finite Fix typo Allow assignment with non-finite but allocated data Fix links in intro Add section on analyzing/manipulating trace files Add Extrae+Archer info Update intro to include Extrae/Paraver Remove setLocation() in Field3D::operator=(FieldPerp) Add helper function for filling fields with function at each point Test all first and second derivatives in X, Y and Z Actually remove the checkData calls Update unit tests for checkData removals ...
Should be done better by actually calculating the arc length of the field line
|
Ok, think this is finally, finally done! |
* next: (57 commits) Make messages(de) more consistent Fix expr.hxx Improve translation Update name pf helper function Add German translation Update generated file Extend README String changes for translation Auto: update libbout.pot Replace IsField*Equal* predicates with templated versions Use nullptr as default Mesh* argument for Laplacian::create Fix bug in DST option for cyclic solve Add initial simple tests for advection derivatives Increase grid size used in test_derivs unit tests Add 'using namespace bout::globals;' for new unit test files. Ignore hidden and temporary files Pass mesh_in in initializer list of FCIMap::FCIMap constructor Pass parameter pack by forwarding-ref, function by const-ref Use bout::globals::dump in BOUTMAIN macro Use bout::globals::dump in SlepcSolver ...
Nothing uses the extra parallel slices yet -- waiting for the derivatives work to be finished.See #1336 for some discussion