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

Remove the need of POIs initialization #80

Closed
federicofernandez opened this issue May 11, 2017 · 7 comments
Closed

Remove the need of POIs initialization #80

federicofernandez opened this issue May 11, 2017 · 7 comments

Comments

@federicofernandez
Copy link
Contributor

federicofernandez commented May 11, 2017

Currently, it is necessary to call an initialization routine when adding POIs to a network:

net.init_pois(num_categories=1, max_dist=2000, max_pois=10)

As documented in the tutorial here

I tracked down the need for this initialization down to the CH code, and it seems that the need comes from the initialization of these data structures.

Then, it seems to be that a way to do that initialization on demand should exist.

It would be great to know @fscottfoti opinion about this.

@fscottfoti
Copy link
Contributor

Yeah but there's an assertion that it can only be called once. Seems like some deeper surgery will be necessary.

@federicofernandez
Copy link
Contributor Author

Difficult to know why it doesn't just returns in case it's already initialized.
Or why it's not called from the first insert.
Perhaps configuration and initialization are done in the same call?

@fscottfoti
Copy link
Contributor

Well you have to tell it how many POI categories you're going to have, so this is the call that does that. Then the first call that you initialize will only initialize one category, and each successive call will initialize a category. It seems to make sense. It'd be nice to be able to change the number of categories after the first step, but that doesn't seem support by CH.

As for the assertion, don't those only exist when compiled in debug mode?

@fscottfoti
Copy link
Contributor

Looks like that we need to solve this in order to solve #73. When I look at the code, I wonder why we can't just reinitialize an element of poiIndexArray, or even make it longer or shorter. Is it more complicated than that? It very well might be - I didn't look too much deeper than that. Remember Dennis just did this work in a few days so maybe he just didn't want to add the methods and do the error checking to modify the array...

@fscottfoti
Copy link
Contributor

Fede, if you look at #81 I started re-initialize the POI indexes in order to solve #73. It seems to work ok, so I think what you were discussing here should work - you shouldn't have to call init_pois.

@federicofernandez
Copy link
Contributor Author

federicofernandez commented May 12, 2017

Right. What you say seems to be right.

As always, with C++ it's often difficult to understand all the container / memory handling until you get to the last line of the code and check that there are no strange allocations, but it doesn't seem to be the case.

In principle, two comments:

  1. The poiIndexArray is a std::vector, so it's totally safe to add / remove / replace objects, as defined here.

  2. The other important point is the class of the elements contained in that structure. Of course, they are of type POIIndexArray. I looked a bit into the constructor and destructor and in principle there's nothing to worry about. The same about the members of the class, there are two std::shared_ptrs that should take care of their memory well, and there is a pointer to the graph that AFAIK won't be an issue at destruction time.

@fscottfoti
Copy link
Contributor

Fixed by #87

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

No branches or pull requests

2 participants