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

[Change] Lots of filtering implemented #287

Merged
merged 6 commits into from
Nov 28, 2018
Merged

[Change] Lots of filtering implemented #287

merged 6 commits into from
Nov 28, 2018

Conversation

lucaspcram
Copy link
Contributor

@lucaspcram lucaspcram commented Nov 28, 2018

Description:

Changes included in this PR:

  1. ChangeAtlas.edge(id) and ChangeAtlas.edges() now filter out edges who are missing one or both of their start/end nodes.

  2. Nodes now filter out connected edges that are present in the source atlas, weren't removed by a FeatureChange, but that have been removed by the algorithm described in 1)

  3. Relations now filter out removed members from their member list, even if a member was indirectly removed thru a FeatureChange that touched a dependent feature (eg. if a FeatureChange removed a node, which triggered an edge removal where the edge was a relation member).

  4. ChangeAtlas.relation(id) and ChangeAtlas.relations() now filter out empty and shallow relations. It handles shallowness at any level of nesting (see unit tests for confirmation).

Unit Test Approach:

Added some new unit tests. See "Files Changed".

Also added a new relation to the test atlas. This relation contains one member, namely another relation. That member relation also contains only relations as members. This is useful for testing that relation shallowness removal works at multiple levels of nesting.

Test Results:

Unit tests pass


In doubt: Contributing Guidelines

@matthieun matthieun changed the title Lots of filtering implemented; see description [Change] Lots of filtering implemented Nov 28, 2018
Copy link
Collaborator

@matthieun matthieun left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks!

}
if (relation.members().isEmpty())
{
return null;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Will that take care of the shallow relation case?

@@ -7,7 +7,7 @@

/**
* Utility class for Change entities: ChangeNode, ChangeEdge, etc.
*
*
Copy link
Collaborator

Choose a reason for hiding this comment

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

Outrageous.

@@ -100,14 +99,11 @@ private RelationMemberList membersFor(final RelationBean bean)
{
final AtlasEntity memberChangeEntity = getChangeAtlas().entity(item.getIdentifier(),
item.getType());
if (memberChangeEntity == null)
if (memberChangeEntity != null)
Copy link
Collaborator

Choose a reason for hiding this comment

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

That would take care of the lower order relations automatically. Thanks!

@@ -242,4 +300,55 @@ public void testRemoveRelationMemberWithoutChangingMemberList()
.firstMatching(member -> "child1".equals(member.getRole())).get().getEntity();
Assert.assertEquals(newMembers.asBean(), fromRelation.members().asBean());
}

@Test
public void testRemoveShallowRelations()
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

@@ -105,4 +105,9 @@
<tag k='name' v='parent' />
<tag k='type' v='parent' />
</relation>
<relation id='-41861' action='modify'>
<member type='relation' ref='-41860' role='child1' />
<tag k='name' v='super-parent' />
Copy link
Collaborator

Choose a reason for hiding this comment

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

super shallow?? ;p

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dang it I forgot about that official terminology :P

@matthieun matthieun merged commit 5c7b978 into osmlab:change Nov 28, 2018
@lucaspcram lucaspcram deleted the lucasChanges branch November 29, 2018 05:21
lucaspcram pushed a commit that referenced this pull request Dec 12, 2018
* [Change] Skeleton for Atlas Change feature (#273)

* test change ideas

* more

* better comments

* test skeleton

* test and more details

* more testing and validation

* refactoring

* validation public

* move around and fix test

* touchup

* code review comments and shallow bloated features and more testing

* added bloated relation

* added test

* added relation testing to edge and node

* fix compilation errors

* finished bloated features and related tests

* added missing change items

* [Change] Finished implementing methods for ChangeAtlas (#280)

* few changes and some tests

* more tests

* add synchronized

* code review comments

* [Change] More ChangeAtlas updates (#284)

* Fixed entitiesFor if an entity was missing

* Checkstyle and comment change

* ChangeNodes now filter out edges which are connected but do not exist

* ChangeNodes now filter out parent relations which do not exist.

* Checkstyle correction in BloatedAtlas interface BloatedEntity, and
FeatureChange comment update.

* Added a ChangeAtlasTest and ChangeAtlasTestRule. Remove these before PR

* Implemented proper bounding for all the bloated entities

* Removed local ChangeAtlasTest

* Fixed spotless errors. but now unit tests are failing

* Fixed a bunch of issues with bounds calculation

* Spotless

* [Change] Fixed failing unit tests (#285)

* Fixed failing unit tests

* Fixed a typo

* [Change] Add validation and related testing to FeatureChange and Change (#286)

* with failing tests

* Validation

* synchronize add and get

* [Change] Lots of filtering implemented (#287)

* Edges now filter themselves out if one of their connected nodes is missing

* Relations now filter out members which do not exist. ChangeAtlas filters empty/shallow relations

* Added a test to demonstrate that shallow relation removal is robust

* Spotless apply

* Added some additional relation tests

* Updated a comment

* [Change] ChangeAtlas validation (#288)

* tests shuffle

* All tests

* spotless

* logger not used

* [Change] Simple refactor and Javadoc (#289)

* compilation and sonar

* forgotten some methods

* package simplification and Javadoc

* forgot tests

* [Change] More complex unit tests (#290)

* add more complex unit tests

* AtlasChangeGenerator

* Added shortcuts for tags

* [Change] Ability to merge FeatureChange objects (#291)

* Added feature change mergeability

* Attempt merge in change

* more tests

* even more tests

* last one?

* [Change] Fixed bug with missing parent relations (#292)

* relation member feature merging broken

* Merge relations in many more places

* Fix tests and change osmIdentifier API in relation

* Added equality tests

* Some cleanup from testing

* Fixed tests and added new relation based generator

* [Change] Keep track of explicitly removed relation members (#295)

* Keep track of explicitly removed relation members

* use method overloading

* Fixed issue where excluded members were wiped in RelationMemberList (#298)

* [Change] Added an AtlasDiff class to produce Changes from a before and after Atlas (#293)

* Started working on a diff generator

* Working on basic AtlasDiff

* Still trying to make valid test case

* Removed relations from test atlas

* Removed node from before atlas

* Still working on diff

* Continuing draft of the AtlasDiff

* Saving temporary working changes

* Lots more doc comments

* Added more comments

* Fixed compilation errors after rebase

* Working on modified entities, more comments

* Working on modify case.

* Lots of new stuff. Doesnt compile lol

* Trying node diffs

* Trying to get parent relations test to work

* Refactor name

* Working on relation support. THings may need to change

* More progress

* getTagChangeIfNecessary now respects useBloated and saveGeo flags. Also fixed a bug in parentRelationIfNec

* Finished member in relation diffing

* Added edge diff

* Added the rest of the changeIfNecessary method

* Relation diffs and more testing

* Finished implementing tests

* Checkstyle and spotless

* Added PackedAtlasCloning equality to tests

* Updated a test

* Tests still failing even after rebase :(

* Isolating bug using testatlas

* Trying to fix bug...

* Further bugfix. Using non-bloated entities still does not work

* Expose bug when using non-bloated entities

* Bug fix in place. Disallowing non-Bloated entities for FeatureChanges. Need to refactor stuff.

* Removed all geometry matching related code. This feature is not necessary due to improved WaySectionProcessor.

* Removed TODOs in AtlasDiff

* Removed useBloatedEntities option

* AtlasDiff now returns an Optional<Change>, and empty Changes are disallowed by the validator

* Fixed Sonar issues in AtlasDiff and AtlasDiffHelper

* Refactored interface for BloatedRelation::withMembers

* Rename variables for clarity

* Fixed awkward copy-pasted typo

* Minor method name refactor

* Fixed a unit test that used a non-Bloated type to create a feature change

* Refactored withMembers to be more null-safe

* Added javadoc

* Removed old method

* Fixed some Sonar issues. Mostly just ignoring false positives (#304)

* Re-arrange NOSONAR locations
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants