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

grass.jupyter: Add save PNG method to Map #2371

Merged
merged 4 commits into from
May 17, 2022

Conversation

wenzeslaus
Copy link
Member

Map from grass.jupyter can now save PNG images. This is easier than using the filename property and it is consistent with TimeSeriesMap.

Map from grass.jupyter can now save PNG images. This is easier than using the filename property and it is consistent with TimeSeriesMap.
@petrasovaa petrasovaa added this to the 8.2.0 milestone May 13, 2022
@wenzeslaus wenzeslaus requested a review from chaedri May 16, 2022 14:36
Copy link
Contributor

@chaedri chaedri left a comment

Choose a reason for hiding this comment

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

It makes sense that we have save method - consistent with the other classes. And this looks good.

One note I have though: I think if you use the filename parameter in the __init__(), you don't need to use save. I'm not sure that's a problem though... the filename parameter still needs to be there for the read_file parameter.

python/grass/jupyter/testsuite/map_test.py Outdated Show resolved Hide resolved
@wenzeslaus
Copy link
Member Author

Thanks @chaedri! I take it as an oversight that we didn't add that already after the change of the API and naming, so that's why the backport label. Here is some additional reasoning for the change partially based on your thoughts.

...if you use the filename parameter in the __init__(), you don't need to use save.

Right and additionally there is the filename property which gives you name of the file if it is temporary file, but save("...") is good for consistency, but it makes the API worse from point of view of Zen of Python (PEP 20): There should be one-- and preferably only one --obvious way to do it.

Now we have three ways, but each has a specific use case. 1) __init__ parameter: You know you want that in the file or you want to use an existing file. 2) save method: Saving is perhaps an afterthought in your workflow or you want to be really explicit about what you are doing (both great for tutorials! but generally too). 3) filename property: You want to use the file further, but it is temporary and you don't want to take care of the file. This is available mostly because the implementation makes it easy to offer and it has potential to make certain workflows faster. You pick the one which suites you or, if you don't care, you pick the one from tutorial which will likely be save and that's perfectly fine.

...the filename parameter still needs to be there for the read_file parameter.

Agreed, it is just needed even if we would strongly encourage other ways. Leaving out read_file and having it default to True when filename would make sense, but I think it is better to keep the mapping of parameters to behavior more explicit.

@wenzeslaus wenzeslaus merged commit 62e17c3 into OSGeo:main May 17, 2022
@wenzeslaus wenzeslaus deleted the jupyter-map-save-image branch May 17, 2022 13:06
wenzeslaus added a commit that referenced this pull request May 24, 2022
Map from grass.jupyter can now save PNG images. This is easier than using the filename property and it is consistent with TimeSeriesMap. Includes test for file existence after saving.
ninsbl pushed a commit to ninsbl/grass that referenced this pull request Oct 26, 2022
Map from grass.jupyter can now save PNG images. This is easier than using the filename property and it is consistent with TimeSeriesMap. Includes test for file existence after saving.
ninsbl pushed a commit to ninsbl/grass that referenced this pull request Feb 17, 2023
Map from grass.jupyter can now save PNG images. This is easier than using the filename property and it is consistent with TimeSeriesMap. Includes test for file existence after saving.
neteler pushed a commit to nilason/grass that referenced this pull request Nov 7, 2023
Map from grass.jupyter can now save PNG images. This is easier than using the filename property and it is consistent with TimeSeriesMap. Includes test for file existence after saving.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants