Skip to content

Implementing LvRegionCoordinator, osm splitting #138

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 28 commits into from
May 17, 2022
Merged

Implementing LvRegionCoordinator, osm splitting #138

merged 28 commits into from
May 17, 2022

Conversation

@sebastian-peter
Copy link
Member

@sebastian-peter sebastian-peter commented Jan 30, 2022

Resolves #137

@sebastian-peter sebastian-peter added the enhancement New feature or request label Jan 30, 2022
@sebastian-peter sebastian-peter self-assigned this Jan 30, 2022
@sebastian-peter sebastian-peter changed the title Sp/#137 lv region coordinator Implementing LvRegionCoordinator, osm splitting Jan 30, 2022
@sonarqubegithubprchecks

This comment has been minimized.

Sign in to view
@sonarqubegithubprchecks

This comment has been minimized.

Sign in to view
@sonarqubegithubprchecks

This comment has been minimized.

Sign in to view
1 similar comment
@sonarqubegithubprchecks

This comment has been minimized.

Sign in to view
@sonarqubegithubprchecks

This comment has been minimized.

Sign in to view
@sebastian-peter sebastian-peter mentioned this pull request Feb 17, 2022
Closed
@sonarqubegithubprchecks

This comment has been minimized.

Sign in to view
@sebastian-peter
Merge branch 'dev' into sp/#137-LvRegionCoordinator
cc7482f 
# Conflicts:
#	gradle/scripts/spotless.gradle
#	src/main/scala/edu/ie3/osmogrid/lv/LvRegionCoordinator.scala
#	src/main/scala/edu/ie3/osmogrid/lv/MunicipalityCoordinator.scala
@sebastian-peter
Adapting to input data provider
07fec94
@sebastian-peter
Cleaning up imports
d0bae97
@sonarqubegithubprchecks

This comment has been minimized.

Sign in to view
@sonarqubegithubprchecks

This comment has been minimized.

Sign in to view
@sebastian-peter
Introduced tests for corrupt input data
331c801 
Simplified ActorStopSupport
@sonarqubegithubprchecks

This comment has been minimized.

Sign in to view
@sebastian-peter
Fixing and improving boundary polygon creation
3136362 
Adding test for LvRegionCoordinator
Adding ScalaDoc
Removing unnecessary imports
@sonarqubegithubprchecks

This comment has been minimized.

Sign in to view
@sonarqubegithubprchecks

This comment has been minimized.

Sign in to view
@sonarqubegithubprchecks

This comment has been minimized.

Sign in to view
@sonarqubegithubprchecks

This comment has been minimized.

Sign in to view
t-ober
t-ober requested changes May 6, 2022
View reviewed changes
Copy link
Contributor

@t-ober t-ober left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a partial review I will continue soon 🙂

@sonarqubegithubprchecks

This comment has been minimized.

Sign in to view
t-ober
t-ober requested changes May 7, 2022
View reviewed changes
Copy link
Contributor

@t-ober t-ober left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot ! This looks very good. Just a few things and questions.

What I was also wondering about: If we split some region along some administrative boundary. What do we do with an administrative system that only lies within the considered area to such a small degree that it would be practically useless to build a subgrid for?

@t-ober
Copy link
Contributor

t-ober commented May 7, 2022

Thanks a lot ! This looks very good. Just a few things and questions.

What I was also wondering about: If we split some region along some administrative boundary. What do we do with an administrative system that only lies within the considered area to such a small degree that it would be practically useless to build a subgrid for?

Now that I think about it we might circumvent the problem (if it proves to be one) by enforcing the choice of considered area by a full administrative level (assuming that the next lower administrative level always is a set of subsets from its superior administrative level).

@t-ober
Copy link
Contributor

t-ober commented May 9, 2022

Think about if the sub administrative boundaries are real partitions

@sebastian-peter @t-ober
Apply suggestions from code review
e505ec9 
Accepting Thomas' suggestions

Co-authored-by: t-ober <63147366+t-ober@users.noreply.github.com>
@sonarqubegithubprchecks

This comment has been minimized.

Sign in to view
@sonarqubegithubprchecks

This comment has been minimized.

Sign in to view
@sonarqubegithubprchecks

This comment has been minimized.

Sign in to view
@sonarqubegithubprchecks

This comment has been minimized.

Sign in to view
@sebastian-peter sebastian-peter force-pushed the sp/#137-LvRegionCoordinator branch from fa1b250 to 03f0877 Compare May 9, 2022 14:37
@sonarqubegithubprchecks

This comment has been minimized.

Sign in to view
@sebastian-peter sebastian-peter requested a review from t-ober May 9, 2022 14:46
@sonarqubegithubprchecks

This comment has been minimized.

Sign in to view
@sonarqubegithubprchecks

This comment has been minimized.

Sign in to view
t-ober
t-ober approved these changes May 17, 2022
View reviewed changes
Copy link
Contributor

@t-ober t-ober left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks 🙂

@sonarqubegithubprchecks
Copy link

sonarqubegithubprchecks bot commented May 17, 2022

Passed

Analysis Details

0 Issues

  • Bug0 Bugs
  • Vulnerability0 Vulnerabilities
  • Code Smell0 Code Smells

Coverage and Duplications

  • No coverage informationNo coverage information (0.00% Estimated after merge)
  • 3 percent duplication0.00% Duplicated Code (0.00% Estimated after merge)

Project ID: edu.ie3:OSMoGrid

View in SonarQube

@t-ober t-ober merged commit b3f052d into dev May 17, 2022
@t-ober t-ober deleted the sp/#137-LvRegionCoordinator branch May 17, 2022 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@t-ober t-ober t-ober approved these changes

@ckittl ckittl Awaiting requested review from ckittl

@larslenssen larslenssen Awaiting requested review from larslenssen

@danielfeismann danielfeismann Awaiting requested review from danielfeismann

Assignees

@sebastian-peter sebastian-peter

Labels

enhancement New feature or request

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Implement LvRegionCoordinator

4 participants

@sebastian-peter @t-ober @ckittl