search

OSGeo / grass

GRASS GIS - free and open-source geospatial processing engine
https://grass.osgeo.org
Other
852 stars 309 forks source link

[Feat] grass.jupyter naming #2283

Closed chaedri closed 2 years ago

chaedri commented 2 years ago

Please describe. With the release of version 8.2, it would be nice to have a relatively stable API for grass.jupyter, the python package making GRASS more accessible in Jupyter Notebooks. This feature request is just to solicit feedback on the current class/method names and gather possible alternatives if necessary.

Describe the solution you'd like Here is an overview of the existing language:

Describe alternatives you've considered Please feel free to comment with alternative ideas or feedback on the existing names.

Additional context You can test out grass.jupyter and the classes listed here in at the following binder links:

neteler commented 2 years ago

Thanks for having this discussion.

I have one (maybe not very obvious) point.

While this name is pretty clear and without hinting at the underlying technology:

  • InteractiveMap creates interactive maps using folium (leaflet library for Python)

these names are less so:

  • GrassRenderer and Grass3dRenderer create static renderings as png images that are embedded in the notebook

I wonder if Grass should be part of the name. Perhaps MapRenderer and Map3dRenderer or the like might be more obvious and omit the name of the underlying software (which is clear anyway), like InteractiveMap or TimeSeries?

Then I am unsure about TimeSeries.* if it "only" does visualizations while not having this in its name:

  • TimeSeries.* ... creating space time dataset visualizations...

Not sure if I was able to express the differences sufficiently. Just my 0.02 cents :-)

petrasovaa commented 2 years ago
  • TimeSeries.animate() displays a GIF animation of the space time dataset
  • TimeSeries.time_slider() displays an ipywidget datetime slider and rendering.

Regarding time_slider(), I think Show() would be more consistent. There seems to be a widgets.Play widget that could be used here for better control? Then we wouldn't need animate(). I think viewing the time series using this play widget is better than showing a GIF, GIFs are good for exporting animation but they are not interactive, the colors are worse so I am thinking something like TimeSeries.gif(filename=None) could be better and primary tool for animating would be the Play widget (if possible).

petrasovaa commented 2 years ago

I wonder if Grass should be part of the name. Perhaps MapRenderer and Map3dRenderer or the like might be more obvious and omit the name of the underlying software (which is clear anyway), like InteractiveMap or TimeSeries?

Map Map3d InteractiveMap TimeSeriesMap

Map is not exactly matching the terminology used in GRASS (raster map, vector map), but it may be clearer for others.

wenzeslaus commented 2 years ago

Notes from a meeting between @chaedri, @petrasovaa and @wenzeslaus:

Base/main name for the display/render classes

Name Evaluation
Map Spatial is the main purpose, even when there are non-spatial things
Plot More associated with non-spatial data
Figure Often refers to a combination of multiple images and text which is not the primary association we want
Monitor Seems to refer to the physical thing or the process these days
Display Seems more interactive and not used much in this context, perhaps too general
Renderer Not a common word, not in dictionary
Plotter Not used in this context
Image Too many images already (IPython, PIL/Pillow)

Map has an disadvantage that an instance of the class cannot be called map because that would shadow a Python build-in function map. However, that issue may be mitigated by using an adjective:

plot = TimeSeriesPlot()
fig = TimeSeriesFigure()
map = TimeSeriesMap()
temperature_map = TimeSeriesMap()

Notably, m would not work either because that would trigger a Flake8 warning and may trigger a spell check warning. The standard way in Python how to deal with shadowing a build-in or keyword is using a trailing underscore, i.e., _map__. That would not work well for examples, but there naming it with some adjective is likely the best option because in cases like notebooks, there will be more than one map.

Another disadvantage of map is that map means the data values mapped on space thing in GRASS. However, map is used in the more cartographic meaning as in Map Display in GUI and default file name for display commands is map.png, so using map in the data display is not a foreign concept either.

Full names for classes

New Name Alternative Original Name Description
Map GrassRenderer Interface to display modules
Map3D Map3d Grass3DRenderer
InteractiveMap (not changed) Simple interface to Folium
TimeSeriesMap TemporalMap TimeSeries Rendering of spatio-temporal datasets

TimeSeriesMap will fit well with a planned class for handling animations or series of data not managed by the temporal framework which can be named SeriesMap, so there is TimeSeriesMap and SeriesMap.

For TimeSeriesMap, the following options were considered: TemporalMap, TemporalSeriesMap, MapSeries, SeriesMap, TimeSeriesMap, StdsMap, STDSMap, SpatioTemporalDataSetMap, SpatioTemporalDatasetMap TimeSeriesPlot, TimeSeriesVisualization, TimeSeriesVis.

Map (2D) API

The 2D map currently makes the filename accessible with a property called filename which is convenient in some cases like further processing. You can also specify name in the constructor which is useful for some advanced scripting. However, to be consistent with TimeSeriesMap and InteractiveMap, a new save method is needed:

nc_map = Map()
nc_map.filename
nc_map.save("myfile.png")

The method will have to convert (or alternatively fail) when file is not the same format as the underlying file.

TimeSeriesMap API

hydro_map = TimeSeriesMap(...)
hydro_map.baselayer.d_rast(map="elevation")
hydro_map.add_time_series(name="hydrology", element_type="strds")
hydro_map.overlay.d_vect(map="roads")
hydro_map.d_legend()
hydro_map.add_timestamp(...)
hydro_map.show()

The save function works with a single GIF as a filename:

hydro_map.save("myfile.gif")

Maybe you can specify format explicitly in the future:

hydro_map.save("myfile.gif", format="gif")
hydro_map.save("mydirectory", format="png")

Generally, format is determined from extension. GIF means single file. PNG means multiple files:

hydro_map.save("mydirectory/myfile.png")
# Now you have mydirectory/myfile_001.png mydirectory/myfile_002.png ...

To enable further use of the files when the file names are generated internally, the function returns the file names:

assert hydro_map.save("mydirectory/myfile.png") == ["mydirectory/myfile_001.png", "mydirectory/myfile_002.png", ...]

Even in the case of a single file, the file name is returned as a string (or maybe Path) so that it can be used in examples in a notebook with IPython Image for display:

Image(hydro_map.save("myfile.gif"))

A future SeriesMap for non-temporal data would have the same API except for the _add_timeseries function which would be perhaps _addseries with a list of rasters or _addrasters(names, labels), _addvectors, _addraster3d(name, slice="horizontally", labels), ...

wenzeslaus commented 2 years ago

Some additional thoughts by @petrasovaa and @wenzeslaus:

Unifying TimeSeriesMap and SeriesMap API

TimeSeriesMap and (theoretical) SeriesMap can have the same function names for adding temporal data and series of data:

temporal_map = TimeSeriesMap()
temporal_map.add_raster_series(name="...", ...)
temporal_map.add_vector_series(name="...", ...)

non_temporal = SeriesMap()
non_temporal.add_raster_series(names=[...], labels=[...], ...)
non_temporal.add_vector_series(names=[...], labels=[...], ...)
temporal_map = TimeSeriesMap()
temporal_map.add_rasters(name="...", ...)
temporal_map.add_vectors(name="...", ...)

non_temporal = SeriesMap()
non_temporal.add_rasters(names=[...], labels=[...], ...)
non_temporal.add_vectors(names=[...], labels=[...], ...)

... represents d.rast and d.vect parameters (this is not addressed in the current API).

wenzeslaus commented 2 years ago

Interested in giving feedback? The current PRs will be merged as is and the new naming will go in later in a separate PR, so there is still time to give feedback on the names and API. On the other hand, the new API will need to be stable in 8.2 which is due soon, so don't postpone! BTW, thumbs up and down and other emojis are a fine reaction.

neteler commented 2 years ago

I would highly recommend to advertise this PR on grass-dev - not all will get the GitHub notifications and consensus is very important here.

BTW: thanks for the great work!

veroandreo commented 2 years ago

I´d like to give an opinion but need a lil time to read carefully and think a bit, hope to do so by this weekend

veroandreo commented 2 years ago

Despite the disadvantages, I like Map. I think this is how we all call cartographic/geographic figures. Now, since for the interactive class we use the adjective, why not using an adjective for the static maps too? Something like:

In this way the use of the word Map seems more consistent to me. My 0.02 cents :)

chaedri commented 2 years ago

Thank you all for your input. After a discussion today, @wenzeslaus, @petrasovaa and I opted for:

We felt that Map and Map3D did not need an adjective (like static) before because they are the base class used in TimeSeriesMap and Interactive map and because it keeps the class name shorter.

I've included these changes in #2305.