Use Mesh::addCoordinates(location) to initialize Coordinates by johnomotani · Pull Request #1372 · boutproject/BOUT-dev

johnomotani · 2018-11-14T16:23:24Z

Instead of creating Coordinates objects when Mesh::getCoordinates is called, require the user to explicitly request them before using staggered fields. Either with REQUEST_LOCATION(location1, ...) macro, which adds coordinates to the global mesh pointer, or by calling Mesh::addCoordinates(location) for each location required.

Fixes #1369.

Staggered grid section of manual and examples/staggered_grid also updated.

I switched to storing the Coordinates in std::unique_ptr rather than std::shared_ptr because we want the Mesh object to be the only owner of the Coordinates. The change might also offer some small performance advantage.

johnomotani · 2018-11-14T16:30:45Z

If we really wanted to avoid changing user code, we could call addCoordinates() from Field3D::setLocation(), since addCoordinates() does nothing if the Coordinates for that location have already been added. But potentially we call setLocation() quite a lot, so I'm inclined to stay with the REQUEST_LOCATION design and keep the (mostly redundant) calls and checks out of setLocation(). Any preferences?

d7919 · 2018-11-16T09:04:56Z

I'm not sure if this should be going into master as it seems to suggest users need to change how they do things, which I'd have thought should only change with a non-bug-fix release. On the other hand it is a route to avoiding a known bug. Any thoughts @bendudson and @ZedThree ?

d7919 · 2018-11-16T09:06:40Z

One possibility is to actually go with the brute force method (generate all possible coordinates during startup -- downside memory consumption) as a temporary bug fix for this release and then use the approach in this PR for next as a way of properly side-stepping the issue for the next release?

johnomotani · 2018-11-16T09:33:21Z

Agree the brute force as a temporary fix for master is a good idea. Would it be OK to put REQUEST_LOCATION into master as well (although it won't be required to call it) so that code can be compatible with both master and next? [is there a word for the opposite of deprecation? 😉 ]

Previously the localmesh was not passed through to the constructor of d2x and d2y in Coordinates::geometry(), so they used the global 'mesh'. Also, when d2x/d2y are read from the mesh, they are at CELL_CENTRE, so we need to interpolate them to the location of the Coordinates.

Could cause bugs, for example in unit tests of BoutMesh where mesh==nullptr.

johnomotani · 2018-11-20T16:38:10Z

I think this is ready now. As @d7919 suggested, this is a temporary work-around to fix the bug without requiring any changes to user code, before #1392 introduces the long-term implementation in next where users will call REQUEST_LOCATION(location) for each staggered location they need. REQUEST_LOCATION is already present in this PR for forward-compatibility, but does nothing because all the locations are already added to coords_map.

I found a couple of small bugs while writing the unit tests for #1392, so I've included the fixes here to go into master, bdb56d8 and 8795aca.

Instead of creating Coordinates objects when Mesh::getCoordinates is called, create them during initialization. As a temporary workaround to maintain backward compatibility, initialize all locations in BoutMesh::load() so that they are always available. Allow the user to explicitly request a location to be available, by calling Mesh::addCoordinates(location) for each location required. This is optional in v4.2 but will be required from v4.3. When Coordinates::geometry() is called, assume that staggered location Coordinates objects need to be recalculated because the user has changed the CELL_CENTRE version. As a temporary workaround, replace geometry() with geometryNoRecalculate() and geometry() now calls geometryNoRecalculate and then recalculates the staggered versions. This means geometry() can be reset back to not recalculating in next (which is OK because we will add the requirement to call addCoordinates() after calling geometry()) without changing the function signature.

Allows Coordinates pointers to be retrieved where necessary in the unit tests.

johnomotani · 2018-11-25T14:07:25Z

I've taken out the REQUEST_LOCATION macro. As a bugfix this PR should 'just work', so I think it's ready to go in?

ZedThree

Sorry John, I hadn't realised this was a bugfix into master. Looks ok, just a few questions

ZedThree · 2019-01-08T17:21:49Z

  GridDataSource *source; ///< Source for grid data

-  std::map<CELL_LOC, std::shared_ptr<Coordinates> > coords_map; ///< Coordinate systems at different CELL_LOCs
+  std::map<CELL_LOC, std::unique_ptr<Coordinates> > coords_map; ///< Coordinate systems at different CELL_LOCs


I'm not sure about this change to unique_ptr -- is there a reason for that?

@ZedThree We were discussing in the thread on #1392 a while ago. I think the conclusion was that unique_ptr makes sense because the Mesh should always keep ownership of the Coordinates objects, but that we should (eventually?) return references rather than pointers. I'm happy to go with whatever you think best on this though; should I change it back?

Also, even if unique_ptr is the thing to do, it is not a bugfix, so should be in #1392 only, not in this PR. I'll take it out of this one.

I think we can even go further and just store values. It was originally stored as a pointer because we have to make the Mesh before making a Coordinates so it had to be initialised to nullptr. But as we're now storing multiple instances in a map, we can initialise the map to empty and hand out pointers for now, and references later.

ZedThree · 2019-01-08T17:29:09Z

 private:
  /// Allocates default Coordinates objects
-  std::shared_ptr<Coordinates> createDefaultCoordinates(const CELL_LOC location);
+  std::unique_ptr<Coordinates> createDefaultCoordinates(const CELL_LOC location);


The implementation of this was deleted so this should go too

ZedThree · 2019-01-08T17:30:03Z

+    // Can always add ZLOW Coordinates, since z-interpolation on Field2D is a
+    // null operation
+    addCoordinates(CELL_ZLOW);
+  }


This logic is in Coordinates::geometry too -- does it need to be here?

I think it does need to be here. This code ensures the Coordinates are created at each location initially. The code in Coordinates::geometry() replaces (and therefore re-calculates) the staggered Coordinates objects after the CELL_CENTRE Coordinates is changed by geometry(). The idea is that #1392 will update things so that here we only create the CELL_CENTRE Coordinates (i.e. lines 845-860 are deleted), the user has to call Mesh::addCoordinates(location) for any needed staggered location, and if the user calls geometry() it has to be before the staggered Coordinates are initialized.

Maybe there is a neater way, but I don't think we want to call Coordinates::geometry() here because it would duplicate work already done when geometryNoRecalculate is called as part of the constructor in addCoordinates(CELL_CENTRE).

Also similarly fix comments in Coordinates::geometry() and BoutMesh::load().

Implementation was already removed from Mesh.

ZedThree · 2019-01-15T16:12:33Z

After quite a bit of discussion, we think this probably isn't the best way forward. The relationship between Mesh and Coordinates is already a bit too complicated.

Instead, let's just not calculate the Christoffel symbols in geometry -- they are the real cause of the bug. We'll move the calculation out into a separate method, which the vector functions can call. This would be a breaking change, but we think it's unlikely to actually affect anyone.

Doing this would neatly sidestep the root cause of the bug you were seeing, and also have other nice side-effects as it would pave the way for removing them entirely (or at least out to a separate structure).

ZedThree · 2019-01-15T16:17:09Z

I think there was also a bug fix for the non-uniform corrections in here -- @johnomotani do you want to put them in another PR?

johnomotani · 2019-01-15T18:53:51Z

I'd agree moving the calculation of the Christoffel symbols could be a good idea, and happy to have separate things for the vector operations, but I'm concerned about G1, G2 and G3. If I understand right, the idea is to make geometry() not need to call communicate(). G1, G2 and G3 are needed by Delp2, Laplace and Laplacian inversion code. We'd need to make sure they are initialized, and if not in geometry() then somewhere else. But I guess what you wrote above about vector functions would apply here too; an initialization function that's called from a differential operator would be allowed to use communicate() because all processors would have to call the operator.

~~Actually,~~ I think geometry() is not the only issue. The Coordinates::Coordinates(Mesh*,CELL_LOC,const Coordinates*) constructor also needs to communicate after interpolating the metric components, otherwise we wouldn't be able to take derivatives of the metric components later. The original cause of the bug was when constructing a new Coordinates object inside getCoordinates() when getCoordinates() wasn't called by all processors simultaneously (because getCoordinates(CELL_YLOW) was first called in a boundary condition).

PS I'll put the bugfix to non-uniform corrections in a separate PR anyway. The fix to BoutMesh in 8795aca is included in #1470 already.

johnomotani added the bugfix label Nov 14, 2018

johnomotani mentioned this pull request Nov 16, 2018

Set location in LaplaceCyclic::solve() and DC() #1367

Merged

johnomotani force-pushed the getCoordinates_fix branch from 3d01892 to 38e1c4e Compare November 16, 2018 10:06

johnomotani added the work in progress Not ready for merging label Nov 16, 2018

johnomotani force-pushed the getCoordinates_fix branch 2 times, most recently from e29f449 to 91d464f Compare November 18, 2018 21:06

johnomotani added work in progress Not ready for merging and removed work in progress Not ready for merging labels Nov 18, 2018

johnomotani force-pushed the getCoordinates_fix branch from 91d464f to aeee283 Compare November 20, 2018 16:18

johnomotani removed the work in progress Not ready for merging label Nov 20, 2018

johnomotani added 2 commits November 20, 2018 16:22

Remove uses of global 'mesh' in BoutMesh::addBoundaryRegions()

8795aca

Could cause bugs, for example in unit tests of BoutMesh where mesh==nullptr.

johnomotani force-pushed the getCoordinates_fix branch from aeee283 to a264ed3 Compare November 20, 2018 16:24

johnomotani mentioned this pull request Nov 20, 2018

Require Mesh::addCoordinates(location) to initialize Coordinates at location #1392

Closed

johnomotani force-pushed the getCoordinates_fix branch from a264ed3 to f43d78c Compare November 25, 2018 13:18

johnomotani changed the title ~~Use REQUEST_LOCATION(location) to initialize Coordinates~~ Use Mesh::addCoordinates(location) to initialize Coordinates Nov 25, 2018

johnomotani added 2 commits November 25, 2018 13:34

Add nullptr entries to coords_map in FakeMesh of unit tests

f528e6a

Allows Coordinates pointers to be retrieved where necessary in the unit tests.

johnomotani force-pushed the getCoordinates_fix branch from f43d78c to f528e6a Compare November 25, 2018 13:37

ZedThree reviewed Jan 8, 2019

View reviewed changes

johnomotani added 3 commits January 9, 2019 20:16

Fix error message, replacing reference to 'REQUEST_STAGGER'

b5b80f2

Also similarly fix comments in Coordinates::geometry() and BoutMesh::load().

Remove createDefaultCoordinates() method from header

0bf16af

Implementation was already removed from Mesh.

Revert change of Mesh::coords_map to use std::unique_ptr

75ce522

ZedThree closed this Jan 15, 2019

johnomotani deleted the getCoordinates_fix branch March 11, 2019 10:02

Uh oh!

Conversation

johnomotani commented Nov 14, 2018

Uh oh!

johnomotani commented Nov 14, 2018

Uh oh!

d7919 commented Nov 16, 2018

Uh oh!

d7919 commented Nov 16, 2018

Uh oh!

johnomotani commented Nov 16, 2018

Uh oh!

johnomotani commented Nov 20, 2018

Uh oh!

johnomotani commented Nov 25, 2018

Uh oh!

ZedThree left a comment

Choose a reason for hiding this comment

Uh oh!

ZedThree Jan 8, 2019

Choose a reason for hiding this comment

Uh oh!

johnomotani Jan 9, 2019

Choose a reason for hiding this comment

Uh oh!

johnomotani Jan 9, 2019

Choose a reason for hiding this comment

Uh oh!

ZedThree Jan 15, 2019

Choose a reason for hiding this comment

Uh oh!

ZedThree Jan 8, 2019

Choose a reason for hiding this comment

Uh oh!

ZedThree Jan 8, 2019

Choose a reason for hiding this comment

Uh oh!

johnomotani Jan 9, 2019

Choose a reason for hiding this comment

Uh oh!

ZedThree commented Jan 15, 2019

Uh oh!

ZedThree commented Jan 15, 2019

Uh oh!

johnomotani commented Jan 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

johnomotani commented Jan 15, 2019 •

edited

Loading