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

added default options to tile_layer #124

Closed
wants to merge 1 commit into from

Conversation

ozak
Copy link

@ozak ozak commented May 12, 2015

This addresses #123

@birdage
Copy link
Contributor

birdage commented May 12, 2015

@ozak can you share an example image or notebook showing the fix pls. 👍

@ozak
Copy link
Author

ozak commented May 12, 2015

Will do once I solve come issues with the projection of the data I have and upload the web site I am working on.

@birdage
Copy link
Contributor

birdage commented May 12, 2015

@ozak great thanks!

@ozak
Copy link
Author

ozak commented May 12, 2015

One question...I have been trying to have the layer be active to start with, which is what the active=True option should accomplish. Looking at some examples it seemed to be just a mater of having a .addTo(map) in the L.tileLayer() command. But that is not the case from some test I just ran. Any ideas?

@birdage
Copy link
Contributor

birdage commented May 12, 2015

@ozak that should do it in my experience, adding add to map should activate the layer auto-magically, can u see the layer in the list?

@ozak
Copy link
Author

ozak commented May 12, 2015

without the change in the active option that I included, the layer is on the list and can be added by clicking. But if I activate it by adding the .addTo(map) like below does not work.

var tile = L.tileLayer('./data/{z}/{x}/{y}.png',{
                  'minZoom': 0,
                  'maxZoom': 18,
                  'tms': true,
                  'continuousWorld': false,
                  'noWrap': false,
                  'zoomOffset': 0,
                  'zoomReverse': false,
                  'opacity': 1 
                  }).addTo(map);

which is what would be generated with the change I made. Or do you mean another folium command?

My test basically executed the following code in an IPython notebook,

map_osm = folium.Map(location=[0,0], tiles='OpenStreetMap', zoom_start=2)
map_osm.add_tile_layer(tile_name='Name',tile_url='./data/{z}/{x}/{y}.png', active=True, tms=True)
map_osm.add_layers_to_map()
map_osm.create_map(path='./webpage.html')
map_osm.render_iframe = True
map_osm

using master the layer is selectable but not active, and using this pull request it doesn't even work, unless active=False.

@birdage
Copy link
Contributor

birdage commented May 12, 2015

@ozak interesting, that would work, maybe its the variable declaration? i.e var tile = try removing that....not hopeful that will work tho

@ozak
Copy link
Author

ozak commented May 12, 2015

Are you able to have some (non-base) layer active to start with?

@birdage
Copy link
Contributor

birdage commented May 12, 2015

@ozak not at the min i dont think, the only things active from init are the base layers

@ozak
Copy link
Author

ozak commented May 12, 2015

It looks like one needs to add it to the map's layers:

var map = L.map('folium_d128601ca2af4b14b77d02ab215b86b9', {
                             center:[0, 0],
                             zoom: 2,
                             maxBounds: bounds,
                             layers: [base_tile, tilelayer]
                           });

@birdage
Copy link
Contributor

birdage commented May 12, 2015

@ozak you shouldn't really need to do that, doing addtomap should add and activate the layer, also if we go the above way it should be part of a layer control group

@ozak
Copy link
Author

ozak commented May 12, 2015

I completely agree with you. It should add it without having to do much else. Looking at the html output and the code, it seems add_layers_to_map() does add the control, but it is not clear if it also adds the layer. from here I thought it would need another addLayer command. Am I misunderstanding Leaflet?

@ozak
Copy link
Author

ozak commented May 12, 2015

Looks like one would need to generate some loop to write the elements of layer_list into the following

map.addLayer(layer_layer_element);

which would have to come after the var map= statement.

@ocefpaf
Copy link
Member

ocefpaf commented May 22, 2015

@ozak Can you add a test to your new functionality? Just a simple template test like we have in https://github.com/python-visualization/folium/blob/master/tests/folium_tests.py would do.

@ozak
Copy link
Author

ozak commented May 25, 2015

Ok...I added the test and it worked on my computer. I see it is failing on 3.3 (passes on 2.7 and 3.4), but it is not related to what I have done. It fails due to

assert self.map.template_vars['fit_bounds'] == fit_bounds_rendered

@ocefpaf
Copy link
Member

ocefpaf commented May 26, 2015

Python 3.x is crashing randomly and I am unsure why. Anyway, I re-started the test and Travis-CI is happy now.

Back to you @birdage! If you think this is ready merge away!

@andrewgiessel
Copy link
Contributor

My guess is that the crashes are related to an issue I unearthed in #152: dictionaries are unordered and thus if you don't use the sort_keys=True flag, then the string you get from json.dumps() has random order and the string comparison fails.

@ocefpaf
Copy link
Member

ocefpaf commented Jul 27, 2015

@andrewgiessel Thanks for fixing the tests.

@ozak Sorry for the delay! Can you squash your commits?

@birdage Are you done with the reviews?

missing template added

Otherwise ``folium.initialize_notebook()`` causes error.

added test
@ozak
Copy link
Author

ozak commented Aug 1, 2015

@ocefpaf Done!

def add_tile_layer(self, tile_name=None, tile_url=None, active=False):
def add_tile_layer(self, tile_name=None, tile_url=None, active=False, minZoom=0,
maxZoom=18, tms=False, continuousWorld=False, noWrap=False, zoomOffset=0,
zoomReverse=False, opacity=1, attribution='Leaflet'):
Copy link
Member

Choose a reason for hiding this comment

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

attribution should be a string about the source of the layer, correct? Leave the default equal to None.

Copy link
Member

Choose a reason for hiding this comment

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

@ozak can you write some docstring regarding these options?

@ocefpaf
Copy link
Member

ocefpaf commented Aug 1, 2015

Thanks @ozak! I made a few comments in your PR. Feel free to ping me if you need any clarification.

@ozak
Copy link
Author

ozak commented Aug 4, 2015

@ocefpaf I'll try to answer to your comments, but won't be able to do so for a couple of days, since I am trying to meet a couple of deadlines. Sorry for the delay!

@ocefpaf
Copy link
Member

ocefpaf commented Aug 4, 2015

No problem. The time we demand from contributors is inverse to pay. That means you get zero pay but infinite time 😜

PS: If you address the comments I might get this merged as is and we can work on it later.

@ozak
Copy link
Author

ozak commented Aug 4, 2015

The problem is that I have to reacquaint myself with the code and leaflet in general... 😄

@ocefpaf
Copy link
Member

ocefpaf commented Aug 4, 2015

The problem is that I have to reacquaint myself with the code and leaflet in general...

I hear you! The whole purpose of folium is to avoid that, but as developer we cannot afford to do it 😒

@ocefpaf
Copy link
Member

ocefpaf commented Aug 18, 2015

@ozak with the recent changes on folium I am certain that this belongs in the v0.1.6 branch. Let me know if you can point this PR there or if I should try some git-fu to cherry pick this.

(See http://stackoverflow.com/questions/24159036/how-to-modify-a-pull-request-on-github-to-change-target-branch-to-merge-into)

@ocefpaf ocefpaf added this to the v0.1.6 milestone Aug 19, 2015
@ocefpaf ocefpaf self-assigned this Aug 19, 2015
@ozak
Copy link
Author

ozak commented Aug 21, 2015

Sure...I will take a look into it. But as I said it might take a while (at least a month) before I can go back to this.

@ocefpaf
Copy link
Member

ocefpaf commented Aug 24, 2015

Not a problem. I am behind the schedule with v0.1.6 (and we can always do a v0.1.7) 😉

@ocefpaf
Copy link
Member

ocefpaf commented Nov 4, 2015

Closing this in favor of #236.

Thanks @ozak! (I kept your commits in #236)

@ocefpaf ocefpaf closed this Nov 4, 2015
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.

None yet

4 participants