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

Geo-point to station conversion #2369

Merged
merged 4 commits into from Feb 26, 2019

Conversation

Projects
None yet
3 participants
@rinkk
Copy link
Member

rinkk commented Feb 18, 2019

Converts geometry points into station objects.

A flag signals if all points should be converted or just the ones not used in any polyline or surface.


void GEOObjects::geoPointsToStation(std::string const& geo_name,
std::string const& stn_name,
bool only_unused_pnts)

This comment has been minimized.

@endJunction

This comment has been minimized.

@rinkk

rinkk Feb 19, 2019

Author Member

✔️

GeoLib::GEOTYPE type,
const std::string &geo_obj_name) const
void GEOObjects::markPointsAsUsed(std::string const& geo_name,
std::vector<bool>& flags)

This comment has been minimized.

@endJunction

endJunction Feb 18, 2019

Member

better to return the flags instead of modifying.

This comment has been minimized.

@endJunction

endJunction Feb 18, 2019

Member

if the function does not modify anything, mark it as const.

This comment has been minimized.

@TomFischer

TomFischer Feb 19, 2019

Member

Minor: Maybe the name of the method could be changed to 'markUsedPoints' that is a tiny bit more readable IMHO.

This comment has been minimized.

@rinkk

rinkk Feb 19, 2019

Author Member

Renamed method and set const.

I can return the flags-array instead of returning it, but wouldn't that mean I'd create a copy of it? I need the vector in the calling method anyway, so if I return it here, I'd have the vector twice?!

if (ply_obj)
{
std::vector<GeoLib::Polyline*> const& lines(*ply_obj->getVector());
for (auto line : lines)

This comment has been minimized.

@endJunction

endJunction Feb 18, 2019

Member

Minor: better to write auto* line forcing the value to be a pointer. Same for surfaces.

This comment has been minimized.

@rinkk

rinkk Feb 19, 2019

Author Member

✔️

return;
}
std::size_t const n_pnts(pnts.size());
std::vector<bool> flags(n_pnts, true);

This comment has been minimized.

@endJunction

endJunction Feb 18, 2019

Member

"flags" could be named somehow more meaningful. A vector of booleans is already saying that these are flags...

This comment has been minimized.

@rinkk

rinkk Feb 19, 2019

Author Member

✔️

markPointsAsUsed(geo_name, flags);

std::unique_ptr<std::vector<GeoLib::Point*>> stations(
new std::vector<GeoLib::Point*>);

This comment has been minimized.

@endJunction

endJunction Feb 18, 2019

Member
    auto stations = std::make_unique<std::vector<GeoLib::Point*>();

This comment has been minimized.

@rinkk

rinkk Feb 19, 2019

Author Member

✔️

{
std::size_t const n_pnts(line->getNumberOfPoints());
for (std::size_t i = 0; i < n_pnts; ++i)
flags[line->getPointID(i)] = false;

This comment has been minimized.

@TomFischer

TomFischer Feb 19, 2019

Member

I would reverse the logic. From the method name I would expect that used points have the value true.

This comment has been minimized.

@rinkk

rinkk Feb 19, 2019

Author Member

renamed the method ✔️

GeoLib::Triangle const& t = *(*sfc)[i];
flags[t[0]] = false;
flags[t[1]] = false;
flags[t[2]] = false;

This comment has been minimized.

@TomFischer

TomFischer Feb 19, 2019

Member

Again, I would reverse the logic.

This comment has been minimized.

@rinkk

rinkk Feb 19, 2019

Author Member

renamed the method ✔️

GeoLib::PointVec const* const pnt_obj(getPointVecObj(geo_name));
if (pnt_obj == nullptr)
{
ERR("Point vector %s not found.", geo_name.c_str());

This comment has been minimized.

@TomFischer

TomFischer Feb 19, 2019

Member

This error message and also the following messages are printed to the terminal. Is there also message/error box for the DataExplorer?

This comment has been minimized.

@rinkk

rinkk Feb 19, 2019

Author Member

added error msg for DE ✔️

std::string name = pnt_obj->getItemNameByID(i);
if (name.empty())
name = "Station " + std::to_string(i);
stations->push_back(new GeoLib::Station((*pnts[i])[0], (*pnts[i])[1],

This comment has been minimized.

@TomFischer

TomFischer Feb 19, 2019

Member

You can use the GeoLib::Station constructor that takes a GeoLib::Point.

This comment has been minimized.

@rinkk

rinkk Feb 19, 2019

Author Member

✔️

@codecov

This comment has been minimized.

Copy link

codecov bot commented Feb 19, 2019

Codecov Report

Merging #2369 into master will decrease coverage by 0.06%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2369      +/-   ##
==========================================
- Coverage   32.58%   32.52%   -0.07%     
==========================================
  Files         528      528              
  Lines       19801    19845      +44     
  Branches     9277     9305      +28     
==========================================
+ Hits         6452     6454       +2     
- Misses      10062    10105      +43     
+ Partials     3287     3286       -1
Impacted Files Coverage Δ
GeoLib/GEOObjects.h 33.33% <ø> (ø) ⬆️
GeoLib/GEOObjects.cpp 27.2% <0%> (-3.62%) ⬇️
GeoLib/Polyline.cpp 36.47% <0%> (+0.6%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f3c97d6...b17357a. Read the comment docs.

@rinkk rinkk force-pushed the rinkk:geo2stn branch from 7058b2c to 0bae282 Feb 19, 2019

@endJunction

This comment has been minimized.

Copy link
Member

endJunction commented Feb 22, 2019

For the main new function in GeoLib it would be better to have a unit tests. Otherwise the code is difficult to maintain.

@rinkk rinkk force-pushed the rinkk:geo2stn branch from 5c5955f to 70ed0b5 Feb 26, 2019

@endJunction endJunction merged commit d17b771 into ufz:master Feb 26, 2019

3 checks passed

continuous-integration/jenkins/pr-merge This commit looks good
Details
deploy/netlify Deploy preview ready!
Details
ufz.ogs #20190226.19 succeeded
Details
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.