Modify

Opened 5 years ago

Last modified 2 weeks ago

#10391 new enhancement

Add support for "not element of" operator (∉)

Reported by: Don-vip Owned by: team
Priority: major Milestone: 19.02
Component: Core validator Version:
Keywords: mapcss spatial operator building hangar aeroway barn Cc: simon04, bastiK, lists@…

Description (last modified by Don-vip)

I'd like to write this validator rule:

/* Many misuses of building=hangar insteaf of building=barn because it's the same word in French */
*[building=hangar][!aeroway]  *[aeroway] {
  throwWarning: tr("{0} not inside {1}", "{0.tag}", "{2.key}");
}

It's not possible yet, is it possible to add !∈ as a new spatial operator (negation of element of)?

Attachments (5)

10391_not_working.patch (5.4 KB) - added by Don-vip 4 years ago.
10391.patch (3.3 KB) - added by qeef 5 months ago.
Add support for "not element of" operator (∉)
0001-Add-support-for-not-element-of-operator.patch (4.0 KB) - added by qeef 5 months ago.
Add support for "not element of" operator (∉)
0002-Remove-right-selector-iteration-when-ELEMENT_OF.patch (3.7 KB) - added by qeef 5 months ago.
Remove right selector iteration when ELEMENT_OF
0001-Add-unit-test-for-element-of-operator.patch (5.3 KB) - added by qeef 5 weeks ago.

Download all attachments as: .zip

Change History (34)

comment:1 Changed 4 years ago by bastiK

Does the following work?

*[building=hangar][!aeroway]  *[aeroway] {
  set hangar_in_aeroway;
}
*[aeroway]!.hangar_in_aeroway {
  throwWarning: ...;
}

comment:2 Changed 4 years ago by Don-vip

I don't know.. but it's quite ugly ;) By the way for consistency we should use (U+2209)

comment:3 Changed 4 years ago by Don-vip

Description: modified (diff)
Summary: Add support for "not element of" operator (!∈)Add support for "not element of" operator (∉)

Changed 4 years ago by Don-vip

Attachment: 10391_not_working.patch added

comment:4 Changed 4 years ago by Don-vip

I tried to implement it but it's not working, why?

comment:5 Changed 4 years ago by Aun Johnsen <lists@…>

Cc: lists@… added

Changed 5 months ago by qeef

Attachment: 10391.patch added

Add support for "not element of" operator (∉)

comment:6 Changed 5 months ago by qeef

Guys, I just attached proposal patch, please comment.

I think that the current implementation of ELEMENT_OF is conceptually wrong because of iterating over right selector.

Also, testing is needed - I only tested the patch on:

area[building]  area[landuse=residential][landuse] {
  throwWarning: tr("Building inside {0}", "{0.tag}");
}
Last edited 5 months ago by Klumbumbus (previous) (diff)

comment:7 Changed 5 months ago by qeef

Sorry for typo in the example css, should be:

area[building]  area[landuse=residential][landuse] {
  throwWarning: tr("Building not inside {0}", "{0.tag}");
} 
Last edited 5 months ago by Klumbumbus (previous) (diff)

comment:8 Changed 5 months ago by Don-vip

Thanks for the patch! Simon, would you like to take a look?

Changed 5 months ago by qeef

Add support for "not element of" operator (∉)

Changed 5 months ago by qeef

Remove right selector iteration when ELEMENT_OF

comment:9 Changed 5 months ago by qeef

Just cleaned up the code.

Commit messages:

  • 0001-Add-support-for-not-element-of-operator.patch​
    Add support for "not element of" operator (∉)
    
    This patch changes the `matches` method of `ChildOrParentSelector` in a
    such way that `ELEMENT_OF` and `NOT_ELEMENT_OF` operators iterate over
    the *left* selector.
    
  • 0002-Remove-right-selector-iteration-when-ELEMENT_OF.patch
    Remove right selector iteration when ELEMENT_OF
    

comment:10 Changed 4 months ago by qeef

bump a little bit

comment:11 Changed 4 months ago by stoecker

Bumping is rude. Nagging is better 😊

comment:12 Changed 4 months ago by stoecker

Milestone: 18.10

comment:13 Changed 4 months ago by michael2402

The test you propose is very slow for large datasets. We should avoid having a O(n²) test in MapCSS.

I think that we should define the semantics of the "element of" operator first before changing how it works. I did not find it anywhere in the source or in the wiki.

comment:14 in reply to:  13 ; Changed 4 months ago by Don-vip

Replying to michael2402:

I think that we should define the semantics of the "element of" operator first before changing how it works. I did not find it anywhere in the source or in the wiki.

See #9516

comment:15 in reply to:  14 Changed 4 months ago by qeef

Replying to Don-vip:

Replying to michael2402:

I think that we should define the semantics of the "element of" operator first before changing how it works. I did not find it anywhere in the source or in the wiki.

See #9516

I would stick with well known definitions like https://en.wikipedia.org/wiki/Element_%28mathematics%29

Current implementation, when the following rule is used for two buildings inside residential area,

area[building]  area[landuse=residential][landuse] {
  throwWarning: tr("Building inside {0}", "{0.tag}");
}

returns selection of one of the buildings and the residential area. In patched version, both buildings are selected.

Can we agree that the current behavior is wrong and the patched is right? If not, I would say that the current behavior or operator is at least misleading.

Replying to michael2402:

The test you propose is very slow for large datasets. We should avoid having a O(n²) test in MapCSS.

Agree. But if you want to check if "element of", I don't see any other option in the worst case. I would be glad to discuss how to speedup the process. Also, I would like to point out the following section of the patch:

 58 +                if (!left.matches(e))
 59 +                    return false;
 60 +                boolean is_element_of = false;
 61 +                // Does exist any element in dataset that contains e.osm?
 62 +                for (IPrimitive p: e.osm.getDataSet().allPrimitives()) {
 63 +                    Environment ne = new Environment(p);
 64 +                    if (!right.matches(ne))
 65 +                        continue;

If or then iterate over the left selector and return if not match. Then check out all primitives (here could be some speedup maybe?) but skip those that does not match the right selector. In other words, more specific mapcss rule is written, faster behavior should be delivered. I haven't test it, so this is just theory.

Last edited 2 months ago by Don-vip (previous) (diff)

comment:16 in reply to:  14 Changed 4 months ago by michael2402

Replying to Don-vip:

Replying to michael2402:

I think that we should define the semantics of the "element of" operator first before changing how it works. I did not find it anywhere in the source or in the wiki.

See #9516

This is no exact semantics. It does not define which elements are allowed as outer and which ones are allowed as inner and when exactly the selector matches.

I was thinking of something like:

  • The selector inner ∈ outer matches every node or area inner as long as it is completely inside an area that matches outer.

We still need to define the parent semantics (e.g. that tags of the outer element are made available as parent)

comment:17 Changed 4 months ago by Don-vip

Milestone: 18.1018.11

comment:18 Changed 3 months ago by qeef

Nagging this time (as advised above).

Any replies to comment:15? Would like to know what should be done to include the patch.

michael2402 wrote:
The selector inner ∈ outer matches every node or area inner as long as it is completely inside an area that matches outer.

I think these are two things mixed:

  1. General operator behavior -- this patch deals with this problem.
  2. Possible inner and outer primitives and their relation using the operator -- the ContainsFinder method visit(...) deals with this problem.


michael2402 wrote:
We still need to define the parent semantics (e.g. that tags of the outer element are made available as parent)

Think that semantics is already defined as already works somehow, so we can talk about the need to redefine it. Or am I misunderstanding something? Anyway, this problem is not related to this patch in my opinion.

comment:19 Changed 3 months ago by Don-vip

Milestone: 18.1118.12

comment:20 Changed 2 months ago by qeef

Bumping again as nagging doesn't work.

Don't get me wrong, I just want to keep the thread alive. I am still interested in the proper functionality of as it allows us to create a MapCSS rule for buildings that are not in a residential area. We would appreciate such a rule while HOTOSM mapping.

comment:21 Changed 2 months ago by stoecker

Priority: normalmajor

Please, there should be a decision in one way or the other. Letting the patch provider hang in the air is not good style.

comment:22 Changed 2 months ago by Don-vip

Agreed. Michael, Simon, can you please look into it?

comment:23 Changed 2 months ago by Don-vip

@qeef I'm sorry this takes this long... As an immediate performance improvement I think instead of iterating on all primitives, we should only iterate on closed ways and multipolygons. This should avoid the creation of thousands of Environment objects for each iterated node / unclosed way.

Otherwise without any feedback from other team members I'm ok with the changes. Could you please add a unit test that covers the new operator?

comment:24 Changed 2 months ago by michael2402

I'm not happy with the patch, mostly due to the performance problems it introduces. We add a linear walk through all primitives for an element-of test. Before the patch, we only searched the elements inside the bounding box, which scales way better.
Using the element of test in drawing will introduce performance problems that way. And validation will take longer.
And we do not do any more multipolygon case handling, this code was probably there for a reason.

Due to a missing clear definition of "element of" I can't really tell if it is correct from a algorithmic point of view.

I think we should first write several unit tests to test that operator (you can use the Mappaint tests if you want something graphical or you can write your own test that tests the different cases of points / ways / multipolygons containing each other).

comment:25 Changed 7 weeks ago by Don-vip

Milestone: 18.1219.01

comment:26 in reply to:  23 Changed 7 weeks ago by qeef

@Don-vip @michael2402 Thanks for reply guys. I will create unit tests first. That's good point that will move us forward.

comment:27 Changed 5 weeks ago by qeef

The patch with unit test for element of operator follows. As this is my first unit test for JOSM, I am not completely sure if it's OK.

Also small question - is it possible to run just one test class/method from ant in JOSM?

comment:28 Changed 3 weeks ago by Don-vip

Milestone: 19.0119.02

comment:29 Changed 2 weeks ago by qeef

So, there are two things I would like to discuss.

  1. The first scenario is in 0001-Add-unit-test-for-element-of-operator.patch (sorry for the name, it should be 0003...):
    • MapCSS search area[building] ∈ area[landuse=residential][landuse] {},
    • two buildings inside residential area (in data_nodist/buildings-in-residential.osm).

The discussion should be about the result of search before and after the 0001-Add-support-for-not-element-of-operator.patch​ is applied. After the discussion, I will dig deeper to understand the multipolygon and performance issues mentioned earlier and fix those.

  1. The second thing is testContains in ChildOrParentSelectorTest.java:
    • MapCSS search node[tag("amenity") = parent_tag("amenity")] ∈ *[amenity] {},
    • scenario in data_nodist/amenity-in-amenity.osm.

I think that the result of MapCSS should be the node with amenity tag set. And that's not happening before 0001-Add-support-for-not-element-of-operator.patch​ is applied, neither after.

Thanks!

Modify Ticket

Change Properties
Set your email in Preferences
Action
as new The owner will remain team.
as The resolution will be set.
to The owner will be changed from team to the specified user.
The owner will change to Don-vip
as duplicate The resolution will be set to duplicate.The specified ticket will be cross-referenced with this ticket
The owner will be changed from team to anonymous.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.