Skip to content
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

Support multiple parallel slices in ShiftedMetricInterp #2049

Merged
merged 5 commits into from
Jun 5, 2020

Conversation

johnomotani
Copy link
Contributor

  • ShiftedMetric has supported MYG parallel slices instead of just one yup and one ydown slice, this PR adds similar support to ShiftedMetricInterp
  • Also replaces use of BoutMask skip_mask with support for Regions. This is an optimization - I tried one simulation which ran ~10% faster (for the full simulation, I haven't done detailed timing on the toFieldAligned/fromFieldAligned parts)
    • Creates a default region in ZInterpolation to use for interpolation with a non-zero y_offset which replaces the previous creation of skip_mask in ShiftedMetricInterp
    • Allows the user to pass some other region in the ZInterpolation constructor. ZHermiteSpline is not totally optimal if passing a custom region, because the calculation of a z-derivative also requires a region, and at the moment RGN_ALL is used for the z-derivative if a custom region was passed.

@johnomotani
Copy link
Contributor Author

It wouldn't be hard to split this into 2 PRs if that's preferred (one for 7a6f211 and a second for
1b9754e). The ShiftedMetricInterp changes are built on a slightly changed API for ZInterpolation, so the second PR would depend on the first one.

@ZedThree
Copy link
Member

ZedThree commented Jun 3, 2020

I guess the Region argument is being passed as a pointer so that the default value can be nullptr?

This worries me a little bit, because storing it as unique_ptr means that it actually takes ownership of the passed pointer, which is not obvious from the interface. I think there's at least two ways to fix this.

One would be to take unique_ptr<Region> arguments instead, which makes it much clearer that the interpolation is going to take ownership. See core guidelines R.32.

Another way would be to take it to just take it by value instead. Then there are two ways to do this, the first is to use the default argument as Region<Ind3D> region = {}, and then check region.size() > 0 to see if it was passed. The other is just have two overloads, one with and one without the region argument. The one without can then construct a default value and call the other one.

Storing an empty Region should be pretty lightweight, I think. It will basically have a couple of empty vectors, so total 6 or 8 pointers, plus a couple of ints in Region. unique_ptr has (almost) zero overhead, but we're not making loads of interpolations and passing them around, so not likely to have a massive impact.

A better option might be std::optional, but that's C++17.

@johnomotani
Copy link
Contributor Author

Thanks @ZedThree, I like the idea of just having a Region member with no pointers! Sorry I forgot to tidy this up before submitting the PR, I'd originally meant to have a unique_ptr argument, but had something wrong (think it might have been a missing & from a const std::string& argument somewhere) and got confused by the compiler error messages with all the templates (unique_ptr argument to the constructor of a class that is created by a GenericFactory subclass...) until I gave up on the unique_ptr argument (but meant to fix it later!).

Don't need unique_ptrs for 'region' because it's possible to have an
'empty' Region already. Replaces not-ideal use of raw pointer Region*
constructor arguments.

/// ZInterpolation objects for shifting to and from field-aligned coordinates
std::unique_ptr<ZInterpolation> interp_to_aligned, interp_from_aligned;

Mesh& mesh;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could store const std::size_t ydown_index = mesh.ystart instead of Mesh& to index parallel_slice_interpolators above? This removes the need to include the mesh header here, although we'd still need in it the .cxx file.

For consistency, might then be nice to have constexpr std::size_t yup_index = 0;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ParallelTransform base class already has a Mesh& mesh member, so I didn't need to add it to ShiftedMetricInterp at all... I think it's still nicer to add yup_index and ydown_index members as you suggested, it's probably nicer to have a const size_t than a Mesh member variable in the hopefully-inlined getWeightsFor* methods.

Removes need for including mesh.hxx in shiftedmetric.hxx, and simplifies
the variables going into the getWeightsForY*Approximation methods, which
we want to be inlined and optimized as they are called within tight
loops.
@ZedThree ZedThree merged commit c94d0e4 into next Jun 5, 2020
@ZedThree ZedThree deleted the shiftedmetricinterp-multipleslices branch June 5, 2020 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants