Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implementation of GocadSGridReader #2316

Merged
merged 14 commits into from Jan 16, 2019

Conversation

Projects
None yet
3 participants
@TomFischer
Copy link
Member

TomFischer commented Jan 11, 2019

  • Tool documentation is online.
  • 3 simple tests are added.
  • For more test we need the permissions of the model creators.

private:
std::array<std::size_t, 3> _grid_coords;
std::array<bool, 8> _affected_cells;

This comment has been minimized.

@endJunction

endJunction Jan 13, 2019

Member

why not a bitset? Or is the usage then to awkward?


} // end namespace MeshLib

#endif /* GOCADNODE_H_ */

This comment has been minimized.

@endJunction

endJunction Jan 13, 2019

Member

#pragma once


DBUG("Creating mesh from Gocad SGrid.");
std::unique_ptr<MeshLib::Mesh> mesh(
new MeshLib::Mesh("GocadSGrid", nodes, elements));

This comment has been minimized.

@endJunction

endJunction Jan 13, 2019

Member

The filename would be a better mesh name.


for (auto tok_it = tokens.begin(); tok_it != tokens.end();)
{
std::size_t id(static_cast<std::size_t>(atoi(tok_it->c_str())));

This comment has been minimized.

@endJunction

endJunction Jan 13, 2019

Member

std::atoi

This comment has been minimized.

@endJunction

endJunction Jan 13, 2019

Member

it looks like id could be const

This comment has been minimized.

@endJunction

endJunction Jan 13, 2019

Member
Suggested change
std::size_t id(static_cast<std::size_t>(atoi(tok_it->c_str())));
std::size_t const id(static_cast<std::size_t>(std::atoi(tok_it->c_str())));
}
else
{
dynamic_cast<MeshLib::GocadNode*>(_nodes[id])

This comment has been minimized.

@endJunction

endJunction Jan 13, 2019

Member

Either check the result of dynamic cast for nullptr, or (because you can prove it) do a static cast.


void GocadSGridReader::createElements(
std::vector<MeshLib::Node*> const& nodes,
std::vector<MeshLib::Element*>& elements) const

This comment has been minimized.

@endJunction

endJunction Jan 13, 2019

Member

The function has no other effects, but to change the elements parameter. Better to return, than to use an output-parameter.

}

std::unique_ptr<MeshLib::Mesh> GocadSGridReader::getFaceSetMesh(
std::size_t face_set_number) const

This comment has been minimized.

@endJunction

endJunction Jan 13, 2019

Member
Suggested change
std::size_t face_set_number) const
std::size_t const face_set_number) const
* Class for calculating the index to given 3d position within the
* structured grid.
*/
class IndexCalculator final

This comment has been minimized.

@endJunction

endJunction Jan 13, 2019

Member

Own file, please.

This comment has been minimized.

@TomFischer

TomFischer Jan 15, 2019

Author Member

Done.

return idx;
}

std::size_t operator()(std::array<std::size_t, 3> const& c) const

This comment has been minimized.

@endJunction

endJunction Jan 13, 2019

Member

Don't repeat the logic of the operator(), call one implementation from another.

This comment has been minimized.

@TomFischer

TomFischer Jan 15, 2019

Author Member

Removed one operator. Use the other one everywhere.


const std::size_t idx(w * (_x_dim - 1) * (_y_dim - 1) +
v * (_x_dim - 1) + u);
return idx;

This comment has been minimized.

@endJunction

endJunction Jan 13, 2019

Member

instead of idx = ..., just return...

Suggested change
return idx;
return w * (_x_dim - 1) * (_y_dim - 1) + v * (_x_dim - 1) + u;
"(%d, %d, %d).",
u, v, w, _x_dim - 1, _y_dim - 1, _z_dim - 1);
return std::numeric_limits<std::size_t>::max();
;

This comment has been minimized.

@endJunction

endJunction Jan 13, 2019

Member
Suggested change
;

#include <logog/include/logog.hpp>

#include "Property.h"

This comment has been minimized.

@endJunction

endJunction Jan 13, 2019

Member

logog include not needed, Property.h goes first.

if (*tok_it == "PROP_NO_DATA_VALUE")
{
tok_it++;
if (!prop.checkID(*tok_it))

This comment has been minimized.

@endJunction

endJunction Jan 13, 2019

Member

the if with pre and post increments is repeated few times. Looks like an easy candidate for a lambda...

{
struct Property final
{
std::size_t _property_id{};

This comment has been minimized.

@endJunction

endJunction Jan 13, 2019

Member

The default initialization is not needed, it's done anyway.

Suggested change
std::size_t _property_id{};
std::size_t _property_id;

This comment has been minimized.

@TomFischer

TomFischer Jan 15, 2019

Author Member

Suggested by clang-tidy.

std::string _property_unit;
std::string _property_data_type;
std::string _property_data_fname;
double _property_no_data_value{};

This comment has been minimized.

@endJunction

endJunction Jan 13, 2019

Member
Suggested change
double _property_no_data_value{};
double _property_no_data_value;
struct Region final
{
std::string name;
unsigned bit{};

This comment has been minimized.

@endJunction

endJunction Jan 13, 2019

Member
Suggested change
unsigned bit{};
unsigned bit;

This comment has been minimized.

@TomFischer

TomFischer Jan 15, 2019

Author Member

Suggested by clang-tidy.

*/

#include <fstream>
#include <iostream>

This comment has been minimized.

@endJunction

endJunction Jan 13, 2019

Member
Suggested change
#include <iostream>
{
return 0;
}
else

This comment has been minimized.

@endJunction

endJunction Jan 13, 2019

Member

drop the else after an if including a return.


The tool was used to convert Gocad stratigraphic grids of the Thuringia
syncline within the INFLUINS project.
{{< bib id="Fischer:2015" >}}

This comment has been minimized.

@endJunction

endJunction Jan 13, 2019

Member

We could include a diagram from an EGU16/17 poster:

A -> B
     |
     \/
D <- C

This comment has been minimized.

@TomFischer

TomFischer Jan 15, 2019

Author Member

Done.


namespace MeshLib
{
enum class FaceIndicator : char

This comment has been minimized.

@TomFischer

TomFischer Jan 14, 2019

Author Member

Compare with docu.

@TomFischer

This comment has been minimized.

Copy link
Member Author

TomFischer commented Jan 16, 2019

@endJunction Thanks for the review! Will rebase and clean history and merge if CI is okay?!

@TomFischer TomFischer force-pushed the TomFischer:GO2OGS branch from 9d3b835 to 219646b Jan 16, 2019

@TomFischer TomFischer merged commit 2a347a0 into ufz:master Jan 16, 2019

2 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
deploy/netlify Deploy preview ready!
Details

@TomFischer TomFischer deleted the TomFischer:GO2OGS branch Jan 16, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.