-
Notifications
You must be signed in to change notification settings - Fork 0
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
IllegalArgumentException caused by historic data for ways #423
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one question from my side.
entities.filter { | ||
case entity: E if entity.metaInformation.isDefined => | ||
entity.metaInformation.exists(_.version.contains(maxVersion)) | ||
case _ => true | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this work if we have two versions of the same entity one with metaInformation and one without?
I think in this case, both versions are returned, which could cause problems. Maybe you could add a test for this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point! I changed this into a logger warning and filter out these entities.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if we have an entity that occures only once but have no metaInformation? Will it be filtered out too? I think this could cause a porblem when generating grids?
Maybe you could count the occurences of all enitity ids and only use this filter for those entites that have multiple occurences.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I checked there is always a version at the entity. So imho this is very unlikely. But sure we can split the entities and filter only those with multiple occurences. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, there are of course options to exclude the metainformation. Maybe we should think about to make them mandatory. Will discuss this tomorrow within the sprint :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@danielfeismann Do you have an update on this part?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly, OSM data that includes multiple versions of the same entity is just broken to me. I'd be fine with just assuming that the OSM data we're handling is coherent. Do we really need the meta information then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with you, @sebastian-peter. Tried it again by using overpass, worked well at that location. So I'm fine with closing this one.
…ic-data # Conflicts: # src/main/scala/edu/ie3/osmogrid/lv/LvGridGeneratorSupport.scala
Closed, since no longer necessary when using overpass. |
resolves #422