grass.jupyter: Use grass.temporal API directly in TimeSeriesMap#7344
grass.jupyter: Use grass.temporal API directly in TimeSeriesMap#7344ninsbl wants to merge 9 commits intoOSGeo:mainfrom
Conversation
|
The failing windows test seems to be due to issues with ctypes in Jupyter. I comes up because of the TGIS import, but in principle that should work also on Windows (and seems to work in the temporal modules). Not sure how to debug / fix this? Is it a glitch in grass jupyter init? |
|
Could you clarify why this is even needed? I briefly read the linked issues, but is there a reason why these issues can't be solved in the tools? We have always had issues with ctypes on Windows, so I am not sure the tests fail, although I didn't check what's wrong. So that's my main hesitation with this. Does this solution have any advantage, e.g. speed? |
Sure. I understand your concerns. Yes, going through the commandline tools adds for temporal modules quite a bit of overhead. t.list, t.info, t.rast.list all have to import the relatively heavy temporal library, and initialize tgis. t.list for example is quite slow from my experience, esp. with more mapsets on the search_path. Yet, this overhead / performance impact would not be a show stopper for going through tools. That overhead will most likely not be very significant when rendering a large number of maps. And if using tools is prefered, I can change the approach. Currently however, t.rast.list cannot be used with a where clause and the gran method (this should probably be addressed there anyway). Also the warning message about the number of granules to return with the gran method should probably be included in t.rast.list too. Using the temporal API made that possible and rather simple. The issues with ctypes is probably due to how jupyter session is initialized? Given that the cli-tools work (and require ctypes as well)? Should not libraries be available in Jupyter as well? Maybe something we should look into anyway, no? Unless, -and I think the case can be reasonably be made - the Jupyter API is aiming at less in-depth coding and thus deliberately more focused on a higher level of programing (mostly using tools and not library functions)... Again, I can go either route: try to dig into the ctypes issue or rather revert to tools, as you prefer... |
|
I see why this is advantageous -- #2688 caused difficulties when creating sliders/GIFs of STDS with satellite imagery. The workaround was to use However, I remember from GSoC that we decided to use the CLI tool because of the ctypes issue. This is unfortunately all I know... So, I can see why this would improve functionality but I don't know anything about solving the ctypes issue. |
|
Given we need to better understand the ctypes issues first, I suggest to start addressing the issues in the tools themselves (because we want that anyway, right?) and perhaps making a smaller PR allowing to use these additions in TimeSeriesMap. Depending on the GSoC selection results, we could task the student with this. Regarding the ctypes issue, my understanding was something like this is needed: But I am not sure whether the ctypes usage is even needed in the temporal API. |
This PR changes TimeSeriesMap in grass.jupyter to use the temporal API directly, instead of through tools.
This change
Things to discuss:
t.rast|vect.listto warn users about the number of granules in the list (in case of method "gran")? It could go into https://github.com/OSGeo/grass/blob/main/python/grass/temporal/temporal_granularity.py or https://github.com/OSGeo/grass/blob/main/python/grass/temporal/abstract_space_time_dataset.py, in a separate PR of course...