Skip to content

Warnings feature - configuration part #5

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

Open
wants to merge 8 commits into
base: add-warnings
Choose a base branch
from

Conversation

radim-asamm
Copy link

@radim-asamm radim-asamm commented Jun 29, 2023

Hello all, this is how I approach the feature of warnings: Here is the configuration part:

  • warnings are specially treated variables defined in routing profiles.
  • warning(s) is an assignment, not expression.
  • warnings are sets of identifiers placed in either node or way contexts.
  • warnings are independent from routing process, they do not affect it.
    Here is a simple flowchart explaining how data flow from the low-level BExpression upwards to RoutingContext
    Selection_999(019)
    I assume there will be specific talks so I keep this intro simple.

Copy link

@janaton janaton left a comment

Choose a reason for hiding this comment

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

Brouter is unknown code base to me ... thus giving only some generic code style ideas / questions.

// ...
// ---context:node
// assign warnings = identifier1&identifier2&identifier3
String[] samples = new String[]{null, "", " ", " ", " w", " w ", "w1&w2", " w1&w2 ", " w1&w1 ", "&&&", " &", "& & & ", "&w1&", "w1,w2", "walk-hike.difficult.very"};
Copy link

Choose a reason for hiding this comment

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

For better readability it would be nice to have input and expected output closer together. E.g. using list of Pair

Copy link
Author

Choose a reason for hiding this comment

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

Will do!

.collect(Collectors.groupingBy(this::isValidWarningToken));
List<String> invalid = validityGroups.get(false);
if (invalid != null) {
System.out.println("Invalid warning identifier(s): " + invalid);
Copy link

Choose a reason for hiding this comment

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

Is this standard approach to log? ... no logging system used at all? :)

// Always returns a set, maybe an empty set
public Set<String> parseWarnings() {
if (warnings == null) {
return new HashSet<>();
Copy link

Choose a reason for hiding this comment

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

[minor] Probably no need to create new instance? Collections.emptySet()

.of(warnings.split(WARNINGS_DELIMITERS_REGEX_PATTERN))
.collect(Collectors.groupingBy(this::isValidWarningToken));
List<String> invalid = validityGroups.get(false);
if (invalid != null) {
Copy link

Choose a reason for hiding this comment

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

Is it desired to continue processing if warning rule is invalid?

Copy link
Author

Choose a reason for hiding this comment

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

I think we want to continue. Another question is: Do we need validation of warning identifiers at all? Explanation: We say for example assign warnings = walk.danger.exposure&walk.danger.slippery&walk.access.seasonal. This or similar expression is hard-coded in many profile files. Now we find there is an error in (possibly complex) implementation of seasonal access warning functionality. Here we can disable warnings for seasonal access all at once. (Ignore it, but log it). That was my reasoning.

@afischerdev
Copy link
Owner

Sorry for the delay but please be patient and let us go step by step.
I played a bit with your first sources.
Here are my questions:

You placed some warning definitions into the sample profile. So at the end you have two sets of warnings.
Why not setup the warnings at global section of profile? E.g.

assign wayWarnings w1&w4
assign nodeWarnings w2&w3

I guess you like to add the warnings conditional way by way? Something like

assign warnings =
  if highway=steps then step5

What happens on negative value? Do you add an else part?

assign warnings =
  if highway=steps then step5
  else empty_warning

Conditional placement doesn't work at the moment, is that correct?

@radim-asamm
Copy link
Author

radim-asamm commented Jul 4, 2023

Yes, some explanation is needed. My fault, my apology.
Two sets of warnings:
Here we say: There are features usually mapped as nodes and we want to issue warning when we see it. Examples: gate, safety chains in mountains (indicating exposure to dizzy height), a ford. Also there are features usually mapped as ways. Examples: Cobblestones in the context of road cycling, mountain paths with seasonal access. I believe placing warnings in node and way context expresses well this inevitable distinction between nodes and ways. But what you suggest is of course possible, perhaps a bit more complex to implement.
Conditional placement, syntax:
A warning is an identifier, nothing more. It does not need to know anything about linked implementation and this implementation is done by users of Brouter server (programmers). There may be some default implementations out of the box - this depends on you, project creators and maintainers. We (Asamm), do not intend to suggest more than a few samples. We may, but we want to keep the changes in the parent project as small as reasonably possible. That means the system is split into configuration part (assing warnings=ford.winter in the routing profiles) and the "detectors" in Java classes. These classes perform the (possibly complex) job of deciding if a warning should be issued or not. Following my silly example: We want to warn bikers about a ford, but only if there is no peak of summer in the part of the world where the route is. Another less silly example: Seasonal access to mountain valleys: We allow routing into currently closed, with warning, textual message like "closed now, opens on the 1st of May". This requires a complex parser similar to tricky parsers of opening hours (I have been there :-)). BTW Graphhopper does not allow routing into currently closed ways and that is very confusing IMHO.
A bit more about "detectors". These will be Java classes implementing some interface. Separated into a package, very loosely coupled to Brouter code. There may be a large, complex parser of seasonal access dealing with OSM inconsistencies, but the connection to Brouter is meant to by done by a few lines of code.
My concerns regarding this approach:

  • If I say processUnusedTags=true in the routing profile, will I see all tags (key, values known to lookup.dat) in my messages?
  • Is it possible to get arbitrary value encoded into routing tiles using something like key=* (everything) in lookup.dat?
  • IMHO some simplification of issued warnings is needed (we do not want 50 identical warnings for cobblestones in a row), but this is just some programming.
  • There may be performance concerns as all messages have to be tested by all "detectors". Like O(messages*detectors). I believe this will not be an issue but I will keep this in mind.

Thanks to your attention, I may be slower to respond this week (off work), but i will.

@afischerdev
Copy link
Owner

Thanks for the clearing.
When warning in profile is 'only' identifier I would recoment to use this simple form in global section:

assign wayWarnings w1;w4
assign nodeWarnings w2;w3

As calling client you know what you want to see as warnings. You don't have to place it all around the profile.
Please notice the new delimiter ';'. This gives you the chance to place the new parameter into the 'extraParams' (please see here. Means you don't have to place it into the profile direct, the extraParams are generated when they are not already defined in profile. You don't need to send a remote profile, just select your favorite profile and add some parameter to it.

Your samples contains a lot of conditional tag - that is not part of the lookups.dat at the moment. So please lets discuss the easy/used tags for now. We will not forget the conditional tags.

If I say processUnusedTags=true in the routing profile, will I see all tags (key, values known to lookup.dat) in my messages?

Yes

Is it possible to get arbitrary value encoded into routing tiles using something like key=*

That is the way it could go. But not in the moment, currently 'only' floating point values are possible.

One more comment:
What do you think about using the json export? It contains all info you need. When you use all profiles with parameter call processUnusedTags=1 you have all defined tags for a way. Just parse it and you have the warnings.

Please see this little step sample
brouter_trekking_0.geojson

I do this in my AFTrack - nearly the sample above.

steps

@radim-asamm
Copy link
Author

Thanks for the suggestions. We do use geojson export. Parsing it client side is something we considered. But the problem is we need to serve 3 client side platforms plus one offline routing solution - so there would have to be 3 versions of a possibly complex functionality, or some multi-platform solution. Also i don't like the notion of configuration being the responsibility of client. (There would have to be 3 tables saying "for walking, warn about this, this, this", all kept in sync with some source of parent-truth). Not a good separation of concern imho. However, there is another way how to achieve what we need without changing brouter code: A proxy or a wrapper, working directly with computed OSMTrack server-side, managing all needed configurations. I will stop developing the solution suggested here for now and explore the other way. Thanks.

@afischerdev
Copy link
Owner

Ok, did you try the csv export in this case?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants