Fix major/minor grid line parameters not working in add_overlay_from_dict #60

lobsiger · 2022-03-20T18:49:35Z

This mainly fixes grid overlays. Wrong key names made major grid lines
always (10, 10) and minor grid lines always (2, 2) whatever the user
script asked for. Added other important keys to kwargs when using agg.
Changed river default level to 2. Level 1 is double lined rivers only.
If a user takes default 1 he will not see much (especially in Europe)
and probably think that adding rivers is broken. Minor typo cleanups.

You asked for it, you got it ...

Closes #xxxx
Tests added
Tests passed
Passes git diff origin/main **/*py | flake8 --diff

always (10, 10) and minor grid lines always (2, 2) whatever the user script asked for. Added other important keys to kwargs when using agg. Changed river default level to 2. Level 1 is double lined rivers only. If a user takes default 1 he will not see much (especially in Europe) and probably think that adding rivers is broken. Minor typo cleanups.

lobsiger · 2022-03-21T12:31:17Z

Please note that this PR should fix satpy bug/issue 1417 :-)

pytroll/satpy#1417

lobsiger · 2022-03-22T08:23:34Z

Here is a test script (rename it from *.txt to *.py and edit 3 path parameters) that lets you try out all possible grid settings:

DIC-test2.txt

lobsiger · 2022-03-23T08:38:56Z

@mraspaud @djhoese it seems a tuple cannot be read from the config file?! Sorry, this goes over my head ...

pnuu · 2022-03-23T10:59:19Z

For most cases lists can be used:

key:
  - list_item_1
  - list_item_2

will be read as:

{'key': ['list_item_1', 'list_item_2']}

pnuu · 2022-03-23T11:00:42Z

Oh, sorry, the config file is in .ini format.

djhoese · 2022-03-23T11:37:07Z

I do not think .ini can do this. The parser which is provided in the standard library has special methods for getting a string versus a float versus an int so no support for more complex structures. It can be added (apparently) with some custom code, but I'm not sure we want to go down that road.

If we wanted to adopt YAML as an alternative I would be OK with that, but we'd still have to have backwards compatibility for .ini for some time.

lobsiger · 2022-03-23T13:03:26Z

@djhoese and @pnuu how about something like:

if 'major_lonlat' in overlays['grid'].keys() or
'minor_lonlat' in overlays['grid'].keys():
major_lon, major_lat = overlays['grid'].get('major_lonlat', (10, 10))
minor_lon, minor_lat = overlays['grid'].get('minor_lonlat', (2, 2))
else:
major_lon = overlays['grid'].get('major_lon', 10)
major_lat = overlays['grid'].get('major_lat', 10)
minor_lon = overlays['grid'].get('minor_lon', 2)
minor_lat = overlays['grid'].get('minor_lat', 2)

It's ugly but should still work with *.ini and this limited parser?!

djhoese · 2022-03-23T13:57:36Z

@lobsiger Be careful who and how you mention people on github. You had put @dave and @Panu but those aren't us, those are other people's usernames (who are now probably getting notifications about this PR, sorry). I've updated your previous comments to use the correct usernames.

As for your solution, it looks fine but I would remove the .keys() part of your if statement. You could also maybe simplify the logic by putting them altogether but I'm not sure it makes the code more readable.

major_lon = overlays['grid'].get('major_lon', 10)
major_lat = overlays['grid'].get('major_lat', 10)
minor_lon = overlays['grid'].get('minor_lon', 2)
minor_lat = overlays['grid'].get('minor_lat', 2)
major_lon, major_lat = overlays['grid'].get('major_lonlat', (major_lon, major_lat))
minor_lon, minor_lat = overlays['grid'].get('minor_lonlat', (minor_lon, minor_lat))

lobsiger · 2022-03-23T18:12:08Z

Quite a mess Dave mentioned before is that pycoast has:

major_lonlat (int, int) 0x before this PR, but see link
minor_lonlat (int, int) 0x before this PR, but see link

https://satpy.readthedocs.io/en/latest/_modules/satpy/writers.html#add_overlay

Dlonlat (float, float) 12x
dlonlat (float, float) 12x
Dlon float 10x
Dlat float 13x
dlon ¨ float 7x
dlat float 7x

lon_major float 2x + 1x in coasts_and_grid.ini
lat_major float 2x + 1x in coasts_and_grid.ini
lon_minor float 2x + 1x in coasts_and_grid.ini
lat_minor float 2x + 1x in coasts_and_grid.ini

Unfortunately I added to this confusing zoo of variables:
major_lon int
major_lat int
minor_lon int
minor_lat int

If we have to look for *.ini backward compatibility I think that this
cannot mean we must retain undocumented variables used in the *.ini?!
I rewrote the code using Dlonlat, dlonlat, Dlon, Dlat, dlon and dlat.

I can even mix integers and floats. I'm used to C where a type is a type. But in Python a type seems to be a Camaeleon. I simplified the code ;-\.

mraspaud · 2022-03-23T22:06:27Z

Just for the record, I'm really happy you joined github @lobsiger, thanks, it helps us a lot!

Tried to make config parser tell 'Dlon' from 'dlon'.

lobsiger · 2022-05-04T09:46:56Z

@mraspaud it seems with merging my PR 59 (branch fix_shp) comes trouble (conflicts) with this PR 60 (branch fix_dict). Is this because my local 'main' branch and maybe also my forked github.com/lobsiger/pycoast main branch is now behind the PyCoast main branch on github.com/pytroll/pycoast? All my other branches fix_dict, fix_cities, usr_shps have now only changes with respect to my 'deprecated' main branch!? All have their own unit tests added as well? What do I have to do now?

lobsiger · 2022-05-04T16:05:30Z

@mraspaud may I ask for another review? Thank you. @djhoese I propose to close PyCoast issue 52. The confusing if font is None: is only for PIL that has a hardcoded default font. Fill is used as text color of PIL labels and now also for agg fonts if specified as font_file + fill + fill_opacity + font_size. @djhoese I wonder whether if specified this way fonts are now also covered by 'cache' ?!

coveralls · 2022-05-04T16:12:58Z

Coverage remained the same at 0.0% when pulling eb67eee on lobsiger:fix_dict into 710e2f2 on pytroll:main.

djhoese

Nice job trying to tackle all of this. I made a couple inline comments for some things that still weren't quite clear to me. I'm not sure that the confusing is None font handling can be removed yet as users could still call add_grid manually outside of Satpy or outside of a dict/config.

I'm also not clear on why this fixes the font behavior in caching? The font object could still be provided by the user, right?

djhoese · 2022-05-05T19:46:58Z

pycoast/cw_base.py

+            if 'major_lonlat' in overlays['grid'] or 'minor_lonlat' in overlays['grid']:
+                Dlonlat = overlays['grid'].get('major_lonlat', (10.0, 10.0))
+                dlonlat = overlays['grid'].get('minor_lonlat', (2.0, 2.0))
+            else:
+                Dlonlat = (overlays['grid'].get('Dlon', 10.0), overlays['grid'].get('Dlat', 10.0))
+                dlonlat = (overlays['grid'].get('dlon', 2.0), overlays['grid'].get('dlat', 2.0))


So am I correct in thinking that this removes the option to use lon/lat_minor/major? So this is a backwards compatibility for users who used this method and the _from_config functionality?

I never used that functionality but I know @mraspaud used it in the past (I think). If we can retain backwards compatibility that would probably be for the best. If we, as a group, can decide on one name and:

Change the positional argument names in add_grid to those names.

Allow for all possible names of the arguments in this from_dict method, but produce a UserWarning or DeprecationWarning when the not-preferred names are used.

That would be amazing. Choosing names that are PEP8/flake8 compatible would be nice too (ex. not Dlonlat).

You can use Dlon, Dlat, dlon, dlat in the *.ini. These are used in the code several times.

Dlon isn't PEP8 as it is a variable name with capital letters in it. And lon/lat_minor/major was used in the .ini files before, right? So for backwards compatibility we should support that?

djhoese · 2022-05-05T20:02:34Z

pycoast/cw_base.py

            if isinstance(font, str):
                if is_agg:
                    from aggdraw import Font
-                    font = Font(outline, font, size=font_size)
+                    font = Font(fill, font, opacity=fill_opacity, size=font_size)


This is backwards incompatible, right? Anyone who was using outline color for the text color will now get a different/wrong color?

Additionally, the add_grid method still uses outline from the command for the font that it creates in _draw_grid_labels.

The problem is upt to now the lon/lat labels always had the color of the grid. You had to generate your font object in the satpy script If you wanted different colors. Backward compatibility is easy to get if 'fill' defaults to 'outline' (instead of 'white').

Ah that's a nice idea. From what I can tell fill isn't used for anything else as far as grids go? If we were to release all of your fixes and feature ideas and then come out with a pycoast 2.0 that breaks backwards compatibility, do you think these keyword arguments should have different names?

djhoese · 2022-05-05T20:02:47Z

pycoast/cw_base.py

            write_text = overlays['grid'].get('write_text', True)
            if isinstance(write_text, str):
                write_text = write_text.lower() in ['true', 'yes', '1', 'on']
-            outline = overlays['grid'].get('outline', 'white')
+            # This should close PyCoast issue #52


No need for this comment in the long term. Please remove it. Thanks.

djhoese · 2022-05-05T20:08:38Z

pycoast/cw_base.py

+            grid_kwargs = {}
+            if is_agg:
+                width = float(overlays['grid'].get('width', 1.0))
+                minor_width = float(overlays['grid'].get('minor_width', 0.5))
+                outline_opacity = overlays['grid'].get('outline_opacity', 255)
+                minor_outline_opacity = overlays['grid'].get('minor_outline_opacity', 255)
+                grid_kwargs['width'] = width
+                grid_kwargs['minor_width'] = minor_width
+                grid_kwargs['outline_opacity'] = outline_opacity
+                grid_kwargs['minor_outline_opacity'] = minor_outline_opacity


So these are agg-specific kwargs that aren't available in the PIL writer?

No, PIL is very limited, width is always 1 and no opacity.

You mean "yes"? PIL is very limited so these options aren't available in PIL, correct?

djhoese · 2022-05-05T20:25:11Z

I also wanted to add @lobsiger that I see you copied over some of the changes from your other branch that was merged into main. It doesn't look to me like you copied all the changes using git, is that correct? Typically the workflow is:

# in a git repository directory with no modified files
git checkout main
# pull changes from the pytroll repository. This assumes your `git remote -v` names the pytroll repository as upstream.
git pull upstream main
# switch back to this in-development branch
git checkout fix_dict
git merge main
git push

If you don't have an upstream remote then you would add it with:

git remote add upstream git@github.com:pytroll/pycoast.git
git fetch

lobsiger · 2022-05-05T21:51:41Z

@djhoese midnight here, I'll go through this tomorrow and commit possible improvements.

lobsiger · 2022-05-06T13:11:54Z

@djhoese as far as I can see I have addressed all your concerns. This should now be fully backward compatible but:

Lets you use 'fill' from dict (or satpy, or *.ini) under agg as has always been documented in the respective doc string(s)
Let you use minor_width, outline_opacity and minor_outline_opacity when you add grids from dict (or satpy, or *.ini)
Lets you use other than major_lonlat=(10, 10), minor_lonlat=(2, 2) grid spacing from dict (and satpy, issue 1417)

So this closes 3 to 5 bugs and has tests added that document the now working features. I cannot fix the failing code_factor.

The 'if font is None:' problem, as explained before, is just used to get a default font hardcoded in pillow if you use PIL. But as you used AGG it failed as AGG seems to have no default font included. 'fill' is used as color by the lowest level PIL text draw function while the respective AGG text draw function does not use 'fill' as different from PIL the font color is part of an AGG font object.

I'm still new to Git but I have found out by myself and updated everything with respect to pycoast/main that includes my PR 59 (resolved confilcts with my added tests). So unless you come up with some severe other concerns I ask you to merge this now.

Regarding an incompatibel and yet undocumented PyCoast 2.0: I think this is not a good idea. You can always dream of building something with 20 ton blocks of Carrara marble, but at the end of the day you have to build your hat with the stones available.

djhoese · 2022-05-06T14:28:34Z

Ok this PR has sat long enough and I think we could go back and forth on what the best options are and how it should be done, but the PR will slowly grow in scope so let's merge this now. Thank you for preserving backwards compatibility. We can work on breaking changes in a future PR if needed. I'm to the point now that I think we just need to merge your PRs and see what doesn't work for users when it is actually in use.

djhoese · 2022-05-06T14:29:44Z

Oh also note that I change the title of this PR. We use the PR titles in our release notes (changelog.md) so I tried to make it a single statement that makes sense to a user not familiar with the problem/code.

lobsiger added 2 commits March 20, 2022 18:11

Ooops, it's style again ...

dfd017b

lobsiger added 2 commits March 22, 2022 16:01

This should fix some tests ...

42e0340

Ooops, I deleted one line too much ...

26a4580

I reduced the confusing zoo of variables ...

55bd578

Just found out that I can use floats for major_lonlat, minor_lonlat too.

39ff255

I can even mix integers and floats. I'm used to C where a type is a type. But in Python a type seems to be a Camaeleon. I simplified the code ;-\.

This was referenced Apr 21, 2022

Fix 'add_cities' to use modern GeoNames data #61

Merged

Add 'coords_ref' option to 'add_points' to specify lon/lat or pixel coordinates #56

Merged

lobsiger added 3 commits May 1, 2022 18:13

Added tests for non default Dlonlat and dlonlat

af379d6

This now also closes PyCoast issue pytroll#52

35138ec

Reset some defaults to make module grid tests happy.

859d8f2

Tried to make config parser tell 'Dlon' from 'dlon'.

lobsiger added 4 commits May 4, 2022 15:21

Upstream changes merged

6ef7860

Flake8 as usual ..

68ef173

More flake8 as usual ..

cf36ddf

Yes, fill is confusing..

fdd2410

djhoese reviewed May 5, 2022

View reviewed changes

Reverted a few things for backward compatibility

eb67eee

djhoese added the bug label May 6, 2022

djhoese self-assigned this May 6, 2022

djhoese changed the title ~~This mainly fixes grid overlays. Wrong key names made major grid lines~~ Fix major/minor grid line parameters not working in add_overlay_from_dict May 6, 2022

djhoese merged commit 224843e into pytroll:main May 6, 2022

djhoese mentioned this pull request May 6, 2022

Add ability to overlay user specific shapefiles from dict #62

Merged

lobsiger deleted the fix_dict branch May 7, 2022 19:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix major/minor grid line parameters not working in add_overlay_from_dict #60

Fix major/minor grid line parameters not working in add_overlay_from_dict #60

lobsiger commented Mar 20, 2022 •

edited by djhoese

Loading

lobsiger commented Mar 21, 2022

lobsiger commented Mar 22, 2022

lobsiger commented Mar 23, 2022 •

edited by djhoese

Loading

pnuu commented Mar 23, 2022

pnuu commented Mar 23, 2022

djhoese commented Mar 23, 2022

lobsiger commented Mar 23, 2022 •

edited by djhoese

Loading

djhoese commented Mar 23, 2022 •

edited

Loading

lobsiger commented Mar 23, 2022

mraspaud commented Mar 23, 2022

lobsiger commented May 4, 2022

lobsiger commented May 4, 2022

coveralls commented May 4, 2022 •

edited

Loading

djhoese left a comment

djhoese May 5, 2022

lobsiger May 5, 2022

djhoese May 6, 2022

djhoese May 5, 2022

djhoese May 5, 2022

lobsiger May 5, 2022

djhoese May 6, 2022

djhoese May 5, 2022

djhoese May 5, 2022

lobsiger May 5, 2022

djhoese May 5, 2022

djhoese commented May 5, 2022

lobsiger commented May 5, 2022

lobsiger commented May 6, 2022

djhoese commented May 6, 2022

djhoese commented May 6, 2022

Fix major/minor grid line parameters not working in add_overlay_from_dict #60

Fix major/minor grid line parameters not working in add_overlay_from_dict #60

Conversation

lobsiger commented Mar 20, 2022 • edited by djhoese Loading

lobsiger commented Mar 21, 2022

lobsiger commented Mar 22, 2022

lobsiger commented Mar 23, 2022 • edited by djhoese Loading

pnuu commented Mar 23, 2022

pnuu commented Mar 23, 2022

djhoese commented Mar 23, 2022

lobsiger commented Mar 23, 2022 • edited by djhoese Loading

djhoese commented Mar 23, 2022 • edited Loading

lobsiger commented Mar 23, 2022

mraspaud commented Mar 23, 2022

lobsiger commented May 4, 2022

lobsiger commented May 4, 2022

coveralls commented May 4, 2022 • edited Loading

djhoese left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djhoese commented May 5, 2022

lobsiger commented May 5, 2022

lobsiger commented May 6, 2022

djhoese commented May 6, 2022

djhoese commented May 6, 2022

lobsiger commented Mar 20, 2022 •

edited by djhoese

Loading

lobsiger commented Mar 23, 2022 •

edited by djhoese

Loading

lobsiger commented Mar 23, 2022 •

edited by djhoese

Loading

djhoese commented Mar 23, 2022 •

edited

Loading

coveralls commented May 4, 2022 •

edited

Loading