Overlapmesh lint algorithm #84
Conversation
…erateOverlapMesh*
- Changed face areas array from DataArray1D to std::vector - Don't recompute face areas at the end of GenerateOverlapMesh - Reduce number of memory allocations
…teGlobalChange/tempestremap into overlapmesh-lint-algorithm
vijaysm
left a comment
vijaysm
left a comment
There was a problem hiding this comment.
Reviewed most of the smaller changes. Pending review on the lint algorithm itself and some other changes. Will do another round of reviews soon
mk/config.make
Outdated
| DEBUG= FALSE | ||
| OPT= TRUE | ||
| PARALLEL= NONE | ||
| PARALLEL= MPIOMP |
There was a problem hiding this comment.
Will this always be ON by default now ? What happens if MPI is not available locally ? For example, with conda builds, we only use serial GNU compilers. Though on second thought, they only use the autotools route
There was a problem hiding this comment.
Will switch back to NONE
src/FunctionTimer.cpp
Outdated
| } | ||
|
|
||
| // Assign start time | ||
| gettimeofday(&m_tvStartTime, NULL); |
There was a problem hiding this comment.
It is more portable to use the high_resolution_clock provided with standard C++11.
https://en.cppreference.com/w/cpp/chrono/high_resolution_clock
There was a problem hiding this comment.
You can still have your FunctionTimer storing context and details on top - but sys/time.h is tricky to include portably.
| AnnounceBanner(); | ||
|
|
||
| #if !defined(TEMPEST_MPIOMP) | ||
| _EXCEPTIONT("TempestRemap was not compiled with MPI: Parallel operation not supported"); |
There was a problem hiding this comment.
If MPI is unavailable, can it still default to running in serial ?
There was a problem hiding this comment.
I figure if you're running GneerateOverlapMeshParallel it should only work if parallelism is enabled.
There was a problem hiding this comment.
Sure that is fair. I guess if someone wants to verify accuracy or look at scalability etc, they would still run on 1 process with MPI enabled anyway, rather than resorting to a MPI disabled build
| /// Return value of lon is in the interval [0.0, 2.0 * M_PI) | ||
| /// </summary> | ||
| void ToLatLonRad( | ||
| double & lat, |
There was a problem hiding this comment.
Should this not be in CoordTransforms.h ?
src/OfflineMap.h
Outdated
| m_iTargetMask = iTargetMask; | ||
| void SetTargetMask(const std::vector<int> & iTargetMask) { | ||
| m_iTargetMask.Allocate(iTargetMask.size()); | ||
| memcpy(&(m_iTargetMask[0]), &(iTargetMask[0]), iTargetMask.size() * sizeof(double)); |
There was a problem hiding this comment.
This is wrong! mask is an int vector. sizeof(double) should be sizeof(int). Same above for source mask also
Bringing overlapmesh-lint-algorithm in line with changes on master
| #if defined(OVERLAPMESH_USE_SORTED_MAP) | ||
| NodeMap nodemapOverlap; | ||
| #endif | ||
| #if defined(OVERLAPMESH_USE_UNSORTED_MAP) |
There was a problem hiding this comment.
Isn't this either or type definition ? If for whatever reason you define both OVERLAPMESH_USE_SORTED_MAP and OVERLAPMESH_USE_UNSORTED_MAP, this will give a compiler error. And it is confusing in general for readability. Probably may be better to use a single macro that you can check with predefined macro int value like below:
#define OVERLAPMESH_USE_SORTED_MAP 0
#define OVERLAPMESH_USE_UNSORTED_MAP 1
#define OVERLAPMESH_NODEMAP OVERLAPMESH_USE_SORTED_MAP
#if OVERLAPMESH_NODEMAP == OVERLAPMESH_USE_SORTED_MAP
//
#elif OVERLAPMESH_NODEMAP == OVERLAPMESH_USE_UNSORTED_MAP
//
#else
#endif
- Needed for overlap mesh to be used in GenerateOfflineMap
…ectilinearMeshFromFile
No description provided.