Coordinate system labelling by johnomotani · Pull Request #1459 · boutproject/BOUT-dev

johnomotani · 2018-12-16T16:50:04Z

As a preliminary to the ShiftToFieldAligned implementation of ParallelTransform, this PR introduces labelling of the coordinate system used by Field3Ds. This is now stored as an enum class COORDINATE_SYSTEM member of Field3D. The main aim is for toFieldAligned() to be able to skip transforming if a field is already field-aligned. I think it's also helpful for record-keeping, Field3Ds as it allows the coordinate system of a Field3D to be kept with the field, both in the C++ code and in output files (where it is saved as an attribute).

In order to be able to get the COORDINATE_SYSTEM for new fields from the ParallelTransform while initializing the ParallelTransform, I've split the initilization of ParallelTransforms into a minimal constructor and an initialize method: in initialize the Mesh has a valid ParallelTransform so ParallelTransform::getCoordinateSystem() can be called.

Uses the changes in #1432, so using region-arguments-tofromFieldAligned as the base (until that PR is merged).

Define enum class COORDINATE_SYSTEM. Add a COORDINATE_SYSTEM member to Field3D and write as an attribute in output files. The default is set by the ParallelTransform but changed to FieldAligned by 'toFieldAligned()' and changed back by 'fromFieldAligned()'.

Derivative functions are templates, so Field2D needs get/setCoordinates methods (even if these are never called).

Initialize ParallelTransform in BoutMesh::load(), so it is almost always available. Creating the FCITransform requires nx/ny/nz to be set in the mesh, so cannot call setParallelTransform() in the Mesh constructor, since it is called before these are initialized.

d7919 · 2018-12-17T14:02:07Z

  T result(localmesh);
  result.allocate(); // Make sure data allocated
  result.setLocation(outloc);
+  if (std::is_base_of<Field3D, T>::value) {


It would be good to avoid special handling for particular field types so would it be possible to ensure all field types can provide this information? (also in the long run we might need to be able to identify if a Field2D is compatible with a Field3D etc.)

I'm not sure what the right design would be. Things to consider:

Field2D and Field3D probably need different labelling - Field2D is the same for Orthogonal and FieldAligned coordinate systems

could have different COORDINATE_SYSTEM enum classes for Field3D and Field2D plus some methods/functions to compare them

or extend COORDINATE_SYSTEM to something like enum class COORDINATE_SYSTEM = {None, FieldAligned, Orthogonal, FieldAligned2D, FCI} where FieldAligned2D would be used for Field2D and compatible with both FieldAligned and Orthogonal (suggestions for better naming welcome if we want to go with a scheme like this)

In the final implementation, Field2D members of Coordinates need coordinate_system setting. Best thing to do might be split initialization with 'uninitialized' ParallelTransform objects constructed early on (so we can getCoordinateSystem() from them), before most of the initialization of Coordinates.

Field2D could have COORDINATE_SYSTEM::AxisSymmetric?

Implementing operator==(COORDINATE_SYSTEM, COORDINATE_SYSTEM) would give us control over comparisons.

d7919 · 2018-12-17T14:04:26Z

+  /// localmesh->getCoordinateSystem() can call
+  /// ParallelTransform::getCoordinateSystem() for fields belonging to the
+  /// ParallelTransform
+  void initialize();


This sort of two stage setup has advantages for a number of things, but it is an important design decision so I think there would need to be quite a bit of input on this from @bendudson, @ZedThree and others.

@d7919 agreed. I did consider calling setCoordinateSystem explicitly for all the fields in each ParallelTransform, but it got quite messy. It could make more sense to put off this kind of redesign, e.g. if we will make ParallelTransforms belong to Coordinates, it would make sense to design the initialization of both together.

For what I need at the moment, I think could get away with reducing the amount of checking: I only really want the checks in to/fromFieldAligned, and it could be OK in the short term to allow some fields to float around with coordinate_system = COORDINATE_SYSTEM::None, especially since they would only be in the ParallelTransforms and are probably only accessed by index rather than with Field3D operations. I'll have a think if there's a sensible, simpler initial version to start with, so we can give longer consideration to something like the implementation here, which is a significant re-design.

I've moved the split initialization into #1463. This PR still seems to work (at least passes the tests) without it. The split initialization is, I think, necessary for all Field3Ds to have their coordinate_system set correctly, but I think the only ones that don't get one set are in ShiftedMetric or FCI and only get accessed via indices, where no check is (or can really be) performed.

I think something like it was going to be more necessary when I was trying to get Field2D to also have a coordinate_system: then all the Field2Ds in Coordinates need to get set properly (which #1463 actually doesn't help with in the end...) because they multiply Field3Ds in the differential operators, etc., so if they had coordinate_system = COORDINATE_SYSTEM::None then checks would fail, or some horrible hacks would be necessary, like preventing copying the coordinate_system from a Field*D which has coordinate_system = COORDINATE_SYSTEM::None.

I would much prefer to avoid two-phase initialisation if possible, as it increases the surface area for potential bugs.

Would moving the parallel transform from Mesh into Coordinates help? I think that's probably where it really belongs anyway.
Another thing that might help would be to use another type instead of Field*s in Coordinates/ParallelTransform that aren't "on" coordinates (though they'd still need to be "on" meshes).

I can't actually remember now exactly what made me think two-phase initialization was necessary. I've added a few commits trying to sort this a different way, adding constructors which take an explicit CoordinateSystem for the ParallelTransform constructors to use. Also attempting to add coordinate_system to Field2D. Still needs testing, probably tweaking a bit too, but I've got to go now. I'll look over it again later, hopefully this week.

d7919 · 2018-12-17T14:07:33Z

+   * Return the coordinate system of this field
+   */
+  COORDINATE_SYSTEM getCoordinateSystem() const {
+#if CHECK > 0


I think this should always throw as it's a sign of an invalid executable and there's no performance penalty (i.e. it's not a check that typically passes in a valid program).

test-yupdown has a custom field-aligned derivative that did not set the coordinate system, causing an exception when checking was turned on.

ZedThree · 2019-01-07T13:51:49Z

+ * created then must not need to know their COORDINATE_SYSTEM, or must set it
+ * explicitly later.
+ */
+enum class COORDINATE_SYSTEM { None, FieldAligned, Orthogonal, FCI };


http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#enum5-dont-use-all_caps-for-enumerators

We're a bit inconsistent here, but I think this should be CoordinateSystem

New constructor for Field3D which sets coordinate_system explicitly, rather than setting from fieldmesh. Allows constructors of ParallelTransform implementations to set coordinate_system for their member Field3Ds before the Mesh has a ParallelTransform member. Also add 'Mesh& thismesh' member of the base ParallelTransform class, so that all ParallelTransform objects keep track of the Mesh that they belong to.

Also move the definition from field.hxx to bout_types.hxx to be more consistent with other enums and enum classes. Need to rename the map for conversion to strings CoordinateSystem->CoordinateSystemMap to avoid a name conflict.

Custom operator to compare CoordinateSystem variables. FieldAligned, Orthogonal and FCI are used for Field3Ds and are all different. For Field2D, Axisymmetric returns true when compared with itself, FieldAligned or Orthogonal because with a ShiftedMetric ParallelTransform Field2Ds are compatible with either Field3D type. Field2Ds can also use CoordinateSystem::FCI, which only matches with FCI.

Zero initialize the Field2D or Field3D variable in Mesh::get() including passing 'this' pointer to ensure all members (fieldmesh, location, coordinate_system) are properly set.

Avoids 'multiple definition' errors due to implementing the function in a header.

dy is needed by the FCIMap constructor and dz by the ShiftedMetric constructor. Read directly from the Mesh data source instead of reading from Coordinates objects. This avoids the need to create the Coordinates objects until the ParallelTransforms have finished being created, so coordinate_system can be set for the member fields of Coordinates. To reduce code duplications, adds a 'getnz()' method to Mesh, which handles the 3 possible ways of specifying dz in input.

johnomotani · 2019-02-15T13:56:11Z

Closing this proposal: we're working on a different approach to tagging fields with coordinate types.

johnomotani added the feature label Dec 16, 2018

johnomotani mentioned this pull request Dec 16, 2018

New shifted metric 2 #1460

Closed

johnomotani added 4 commits December 17, 2018 13:51

Add coordinate_system to swap(Field3D,Field3D)

5fb0d7d

Set CoordinateSystem of result in derivatives

a1699f6

Derivative functions are templates, so Field2D needs get/setCoordinates methods (even if these are never called).

d7919 reviewed Dec 17, 2018

View reviewed changes

johnomotani force-pushed the coordinate-system-labelling branch from 277d43b to 5e236f3 Compare December 17, 2018 14:04

d7919 reviewed Dec 17, 2018

View reviewed changes

Fix coordinate system tracking in test-yupdown

15e40bb

test-yupdown has a custom field-aligned derivative that did not set the coordinate system, causing an exception when checking was turned on.

johnomotani force-pushed the coordinate-system-labelling branch from 0a13ba4 to 15e40bb Compare December 17, 2018 16:02

johnomotani mentioned this pull request Dec 17, 2018

Separate initialization of ParallelTransform objects from constructors #1463

Closed

johnomotani mentioned this pull request Jan 4, 2019

getShifted/getUnshifted methods for BoutOutputs #1175

Closed

ZedThree reviewed Jan 7, 2019

View reviewed changes

johnomotani added 8 commits January 7, 2019 15:07

Rename COORDINATE_SYSTEM enum class to CoordinateSystem

12b0621

Also move the definition from field.hxx to bout_types.hxx to be more consistent with other enums and enum classes. Need to rename the map for conversion to strings CoordinateSystem->CoordinateSystemMap to avoid a name conflict.

Initialize 'var' in Mesh::get(Field2D), Mesh::get(Field3D)

9394b9d

Zero initialize the Field2D or Field3D variable in Mesh::get() including passing 'this' pointer to ensure all members (fieldmesh, location, coordinate_system) are properly set.

Implement CoordinateSystem for Field2D

90f748d

Add inline keyword to compareCoordinateSystems function

6b55fe4

Avoids 'multiple definition' errors due to implementing the function in a header.

Set coordinate_system for local variables in FCIMap constructor

d98a003

johnomotani force-pushed the coordinate-system-labelling branch from 415065a to 467af37 Compare January 7, 2019 23:27

johnomotani closed this Feb 15, 2019

johnomotani mentioned this pull request Feb 15, 2019

New shifted metric 2: coordinate system labelling #1465

Closed

johnomotani deleted the coordinate-system-labelling branch March 11, 2019 10:02

Navigation Menu

Uh oh!

Coordinate system labelling - #1459

Coordinate system labelling#1459
johnomotani wants to merge 13 commits into
region-arguments-tofromFieldAlignedfrom
coordinate-system-labelling

johnomotani commented Dec 16, 2018 •

edited

Loading

Uh oh!

d7919 Dec 17, 2018

Uh oh!

johnomotani Dec 17, 2018

Uh oh!

ZedThree Jan 7, 2019

Uh oh!

d7919 Dec 17, 2018

Uh oh!

johnomotani Dec 17, 2018

Uh oh!

johnomotani Dec 17, 2018

Uh oh!

ZedThree Jan 7, 2019

Uh oh!

johnomotani Jan 7, 2019

Uh oh!

d7919 Dec 17, 2018

Uh oh!

ZedThree Jan 7, 2019

Uh oh!

johnomotani commented Feb 15, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

johnomotani commented Dec 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johnomotani commented Feb 15, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

johnomotani commented Dec 16, 2018 •

edited

Loading