Require Mesh::addCoordinates(location) to initialize Coordinates at location - #1392
Require Mesh::addCoordinates(location) to initialize Coordinates at location#1392johnomotani wants to merge 27 commits into
Conversation
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.
| Get the Coordinates object of this mesh | ||
| Deprecated version of getCoordinates | ||
| """ | ||
| print("Warning Mesh.coordinates is deprecated, and does not handle " |
| } | ||
|
|
||
| /// switch to pass to Coordinates objects | ||
| bool allow_geometry_without_recalculate_staggered; |
There was a problem hiding this comment.
Please could the doxygen comment be a bit more descriptive?
Also should this be a member of mesh/does it need to be stored at all? It looks like it should maybe just be an argument to Coordinates::geometry?
|
I'm afraid I'm not 100% sure about the utility of the macro. It's not much shorter than Would an alternative to requiring users to call this in their physics models be to put the call to |
|
I'd be fine with getting rid of the macro if you think it's not useful. Why in |
6dec2de to
bbfb58d
Compare
|
Fabio just made a good point: the way this PR has gone, we're now initializing settings for staggered grids in two ways - with the This might be a good time to simplify the initialization. We could either:
In either case I think the Thoughts? I'm inclined to option 2, a compile-time 'setting' in |
|
I would also prefer option 2.
It can still be turned into a run-time switch, if that is of interest.
The user code anyway need to check whether staggering is enabled, before
trying to set a staggered location.
|
bbfb58d to
7e7fa88
Compare
johnomotani
changed the title
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.
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.
Remove initialization of Coordinates for all staggered locations in BoutMesh::load(). User is now required to initialize the Coordinates for any needed locations using Mesh::addCoordinates(location). Removes recalculation of staggered Coordinates in Coordinates::geometry(). The user should instead call addCoordinates(location) after the CELL_CENTRE Coordinates object is updated and its geometry() method is called. geometry() checks that either it has been called at CELL_CENTRE and CELL_CENTRE is the only entry in coords_map, or that the location of the current Coordinates object that called geometry() has not yet been added to coords_map; this check can be overridden by setting the option mesh:allow_geometry_without_recalculate_staggered=true so that the staggered Coordinates objects can be explicitly updated if necessary. If Mesh::addCoordinates(location) is called without explicitly asking for the Coordinates at location to be replaced (using the replace_coords argument) then throw an exception if location is already in coords_map.
tests/integrated/test-drift-instability, tests/MMS/wave-1d and tests/MMS/wave-1d-y use staggered grids and so need to call Mesh::addCoordinates(location).
Move StaggerGrids option into [mesh] section of input files. Call mesh->addCoordinates(CELL_YLOW). Only use CELL_YLOW if mesh->StaggerGrids=true to avoid exceptions.
Need to call Mesh.addCoordinates(location) to initialize staggered grids in tests/MMS/derivatives3 and tests/MMS/upwinding3.
Unit tests for addCoordinates() methods.
Call to addCoordinates('YLOW') is now needed to allow 'YLOW' location to
be used.
Make the allow_geometry_without_recalculate_staggered flag an argument to Coordinates::geometry() rather than an option that has to be read by Mesh and passed to Coordinates.
If Field3D was created in global scope, its fieldmesh pointer is null. getMesh() returns the global mesh in that case, so use getMesh() instead of fieldmesh to avoid segfaults.
7e7fa88 to
53f780a
Compare
| 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 |
There was a problem hiding this comment.
I'm not sure about changing this to unique_ptr. I know we currently hand out a raw pointer, but that should change, and returning a unique_ptr says "I'm giving ownership over to you", whereas a shared_ptr says "You can have a look at this, but I will keep hold of it too"
There was a problem hiding this comment.
My thought in changing to unique_ptr it was that the Mesh object should keep ownership of the Coordinates objects, and not share ownership (since there is no point keeping the Coordinates after the Mesh has been deleted anyway). I'm having trouble finding much discussion, but see e.g. https://www.reddit.com/r/cpp/comments/6dyq6l/is_it_alright_to_return_raw_pointer_from_unique/. Maybe raw pointer is OK, or we could return reference?
|
I think if the result can't be null, and we're not transferring or sharing ownership, then a reference would be the right way to go. I may have got that wrong though. |
|
If in the long term we want to return a reference, what should we do now? Changing |
|
Since coordinates only recently changed to getCoordinates, perhaps getCoordinates could return a reference, and coordinates return a pointer? |
I'd be happy with that, but technically it changes the v4.2 interface... Any thoughts @ZedThree? |
Also similarly fix comments in Coordinates::geometry() and BoutMesh::load().
Implementation was already removed from Mesh.
This member variable was used for reading an option, but the interface was changed to use a method argument passed by the user instead of an option.
|
Interface question: Rather than requiring users to call |
Using std::unique_ptr for Mesh::coords map requires a few changes to FakeMesh.
|
I think we're passing this up in favour of |
4 participants
This is the 'final' version of changes started in #1372. Now only CELL_CENTRE coordinates are created by default, and the user must call REQUEST_LOCATION(location) or mesh->addCoordinates(location) to initialize Coordinates for staggered locations.
Includes updates to the manual, unit tests for
addCoordinates()as part ofBoutMeshTestand support forgetCoordinates/addCoordinatesin boutcore.This does add a couple of methods to
Mesh,hasCoordinates(location)andcountCoordinates(). I think they're justified, but alternatives could be to makeCoordinatesa friend ofMesh, or to make coords_map a public member; I don't favour either of those alternatives, but maybe there are others?