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

Expose get_all method in Multimap interface and provide factory method for Multimap #268

Open
wiktorn opened this issue Sep 1, 2018 · 1 comment

Comments

@wiktorn
Copy link

wiktorn commented Sep 1, 2018

I attempted to implement fix for osmcode/pyosmium#66 but I'm missing few constructs in libosmium.

  1. Factory methods for Multimaps. There are few approaches that I considered, but neither is without drawbacks:
  • just copy&paste most of MapFactory to Multimap
  • create generic MapFactory that can register and create both Map and Multimap by extending the template with one more argument, but this will break backward compatibility
  • create generic MapFactory as a "meta-template", and create separate factories for Map and MultiMap
  1. Multimap interface doesn't specify get_all method (not all implementing classes implement this) so there is no way to retrieve data using Multimap interface, user would have to bind to specific implementation

  2. (probably not that important) Multimap and Map miss iterator, begin, end triple to make it easy to expose a way to iterate through the (key, value) pairs

  3. multimap/all.hpp is missing multimap/hybrid.hpp #include

@joto
Copy link
Member

joto commented Nov 1, 2018

The Multimaps are somewhat neglected area in Osmium. They are not well documented, not well tested, and, as you noticed, incomplete. You have to know exactly what you are doing to use them correctly and efficiently. I am not sure it even makes sense to expose the Multimaps in a generic way to Python. Maybe it would be better to expose only a specific one or just a few of them.

With the current libosmium I recommend using the FlexMem map for almost all use cases, maybe we need a FlexMultiMap first and can just expose that. In any case I'd want more tests before I would start to refactor or extend this code.

To your points:

  1. I think just copying and then changing the MapFactory would be the easiest approach. We definitely want to keep MapFactory and MultiMapFactory separate things. But, as mentioned above, I am not convinced yet that we even need the factory.
  2. I can't remember why get_all isn't implemented in some classes. We'd have to check whether this is possible at all and how to handle the case when it isn't implementable.
  3. We have to see whether this can be implemented in all classes.
  4. I am not sure the all.hpp is that useful. Maybe we should deprecated use of that file.

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

No branches or pull requests

2 participants