Conversation
davschneller
force-pushed
the
davschneller/precommit
branch
from
0a7b018 to
0792a70
Compare
| { | ||
| delete[] m_edges; | ||
| } | ||
| Graph::~Graph() { delete[] m_edges; } |
There was a problem hiding this comment.
Not for this PR, but we should think about eventually making m_edges some kind of standard container.
| private: | ||
| class Graph { | ||
| private: | ||
| counter_t* m_edges; |
There was a problem hiding this comment.
Not for this PR, but we should think about making these pointers unique or shared pointers sometime in the future.
There was a problem hiding this comment.
Yup; the tools (usually; same for the SeisSol main repo for now) usually have a worse code quality I'm afraid. Or they're hopelessly outdated.
We might need to apply clang-tidy onto every project here; maybe even add tests (e2e, or maybe unit), or at least check that they compile.
| * | ||
| * @author Carsten Uphoff (c.uphoff AT tum.de, http://www5.in.tum.de/wiki/index.php/Carsten_Uphoff,_M.Sc.) | ||
| * @author Carsten Uphoff (c.uphoff AT tum.de, | ||
| * http://www5.in.tum.de/wiki/index.php/Carsten_Uphoff,_M.Sc.) |
There was a problem hiding this comment.
Do we also want to make the licenses of this repo similar to what you did with SeisSol?(again in future PRs)
There was a problem hiding this comment.
Sure. I'm all in for it. (there's a currently disabled REUSE check in the pre-commit config anyways—that could be activated while doing that)
There was a problem hiding this comment.
It should not be a problem most likely, but if we could verify that the changes in spaces here is not causing any unknown/weird behavior, if would be good.
| => 6000 m/s BedrockVelModel(5,:) = (/-12d3,2860d0,41298400000d0,41984800000d0/) | ||
| => 6600 m/s BedrockVelModel(6,:) = (/-23d3,3050d0,46390500000d0,60969500000d0/) | ||
| => 7100 m/s BedrockVelModel(7,:) = (/ -5d10, | ||
| 3330d0,65942325000d0,81235350000d0/) => 8000 m/s */ |
There was a problem hiding this comment.
Can we skip linting here.. the units being shifted to next line is kinda making it look weird..
| // << "\" Dimensions=\"" << totalSize[0] << "\">" | ||
| // << | ||
| // backends::Backend::dataItemLocation(backendPrefix.c_str(), "partition") | ||
| // << "</DataItem>" << std::endl; |
| << "\">" | ||
| << std::endl | ||
| // This should be UInt but for some reason this does not work with | ||
| // binary data |
| ----------- | ||
| The tools does only work if faces with free surface boundary condition are present and if their normals look in +z or -z direction. (This is usually the case if free surfaces due to topography and e.g. enu, ned, seu axis orientations are used.) That is, the tool fails if all respective faces have normals with z=0. | ||
|
|
||
| The tools does only work if faces with free surface |
There was a problem hiding this comment.
Sorry for nitpicking but grammar..
vikaskurapati
left a comment
vikaskurapati
left a comment
There was a problem hiding this comment.
Reviewed just because @jwjeremy does not probably have write access? Most of the comments are clarification, or questions about future. But it will be nice to once check if the changes are still stable w.r.t the CAD models mainly. Otherwise LGTM.
|
Hmm; yes that looks like it. @jwjeremy sent you an invite to the SeisSol org |
3 participants
Apply a pre-commit config; organize the readmes, and format all C++ files.
Python files are not yet reformatted (we might need to agree on a style); but we could add it to this PR.