Validate declarative interface attributes

[sgdecker]

This PR addresses a portion of #2016

Right now, typos in the names of attributes used by the declarative
plotting interface lead to strange errors that can be difficult to
debug. Here, a new mixin class is defined to provide
validation of attribute names against a whitelist.

This approach was inspired by a Stack Overflow question
and the nearest-string function was stolen from Rosetta Code.

Since this is a proof of concept, validation is only applied to the
MapPanel object.

Validation is extended to all publicly facing objects in the decalarative interface, and a test has been added.

Here is a motivating example:

from datetime import datetime, timedelta

from metpy.io import GempakGrid
from metpy.units import units
from metpy.plots import ContourPlot, MapPanel, PanelContainer

init_time = datetime(2021, 10, 21, 12)
plot_time = init_time + timedelta(hours=12)

gem_file = '/ldmdata/gempak/model/nam/21102112_nam211.gem'
gem_data = GempakGrid(gem_file)

ht500 = gem_data.gdxarray(parameter='HGHT',
                          date_time=plot_time,
                          level=500)[0] * units('m')

cp = ContourPlot()
cp.data = ht500
cp.contours = list(range(0, 700, 6))
cp.linecolor = 'green'
cp.clabels = True
cp.plot_units = 'dam'

panel = MapPanel()
panel.area = [-130, -70, 20, 55]
panel.layers = ['states', 'coastline', 'borders']
panel.title = f'NAM forecast of 500-mb Heights at {plot_time}'
panel.pots = [cp]

pc = PanelContainer()
pc.size = (12, 10)
pc.panels = [panel]
pc.show()

Output w/o the PR:

Traceback (most recent call last):
  File "/home/decker/classes/met433/new_labs/lab07/typo.py", line 33, in <module>
    pc.show()
  File "/home/decker/src/git_repos/metpy/src/metpy/plots/declarative.py", line 594, in show
    self.draw()
  File "/home/decker/src/git_repos/metpy/src/metpy/plots/declarative.py", line 581, in draw
    panel.draw()
  File "/home/decker/src/git_repos/metpy/src/metpy/plots/declarative.py", line 837, in draw
    self.ax.set_extent(self.area, ccrs.PlateCarree())
  File "/home/decker/src/git_repos/metpy/src/metpy/plots/declarative.py", line 809, in ax
    self._ax = self.parent.figure.add_subplot(*self.layout, projection=self._proj_obj)
  File "/home/decker/src/git_repos/metpy/src/metpy/plots/declarative.py", line 744, in _proj_obj
    if isinstance(self.plots[0].griddata, tuple):
IndexError: list index out of range

Output w/ the PR:

Traceback (most recent call last):
  File "/home/decker/classes/met433/new_labs/lab07/typo.py", line 28, in <module>
    panel.pots = [cp]
  File "/home/decker/src/git_repos/metpy/src/metpy/plots/declarative.py", line 558, in __setattr__
    raise AttributeError(msg)
AttributeError: 'pots' is not a valid attribute for <class 'metpy.plots.MapPanel'>. Perhaps you meant 'plots'?

[sgdecker]

The test works on my machine. Not sure what module is not being found. I suppose it might be the lack of a needs_cartopy decorator, so I'll give that a shot.

[kgoebber]

Thanks for this PR @sgdecker, this has definitely been a pain point for users and would be great to give better error statements to help the users find the right path, rather than either an indecipherable error or silently fail to do what the user expected.

A couple of suggestions including not using the term 'whitelist', I've suggested an alternative, but there are other potential options as well. In addition, there is a function from difflib that might work to our advantage here instead of a custom definition.

[dopplershift]

Ping @dcamron since you worked on another shot at this, at least on the disallowing unknown attributes side.

[kgoebber]

Looking good and glad the built-in function works well - it does simplify the code a lot. I'm wondering if we could add one more test with some out there traitlet to test the else portion of the if statement? Something like panel.aaaaaaa.

[sgdecker]

Looking good and glad the built-in function works well - it does simplify the code a lot. I'm wondering if we could add one more test with some out there traitlet to test the else portion of the if statement? Something like panel.aaaaaaa.

Great idea. I have added a second test and made the original test more specific.

[kgoebber]

Looks good on my end now. This will be a definite improvement for the users. (Don't know why the Docs are failing on 3.9)

[dcamron]

I've come around to reviewing this, and I think the implementation is mostly solid. However, I did just arrive at a thought to ask @kgoebber @dopplershift @sgdecker: do we actually want this hard-and-fast-disallow behavior? Would it be still helpful but more flexible to check and only warn on setting "invalid" attributes instead of outright refusing them?

[kgoebber]

The reason I am in favor of a hard-and-fast disallow is that if you slight mistype an attribute (say by adding an extra s to an attribute name) there are numerous times you get silent failures (still get a map produced) and many newer programmers are less likely to read a warning message if a map still gets produced. The hard disallow would force them to read the error message to then successfully produce a map and since we give a direct suggestion (in what would likely be the vast majority of cases) then the end user should be able to easily correct the error.

[sgdecker]

I agree with @kgoebber . If there was some use case where assigning to additional attributes would make sense, I could see a warning being OK, but I can't see one. A great example of a silent failure one of my students recently experienced, was where he wrote something along the lines of
contours.units = 'dam'
which produced no contours (the request was for a range between 400 and 600 or so) but created a map with some other elements plotted. It was a mystery why the height contours weren't appearing. Certainly a warning would have been better than nothing, but the error forces you to see what the problem is.

In cases where the incorrectly named attribute does trigger an error already (like the original example), I would be worried about a warning being lost among a sea of text from a long traceback message.

[dopplershift]

I'm in agreement with the above. The only use case for a user adding their own attributes (which wouldn't trigger any behavior on their own) is some kind of weird monkey-patching (like we do with .units and DataFrame). I think for the intended use case of the simplified plotting interface we don't need to support this. One can always get there by subclassing instead. I also think a hard error will be much more helpful for our user base than ignorable warnings.

[dcamron]

Thanks for this @sgdecker. I have some thoughts about how we can reorganize this a bit to make Declarative more robust in the future, but I want to get those thoughts to you with some time to respond. We definitely prefer to get this in with the release we want to cut this week! I will open up an issue capturing some of my thoughts and we can try to get that in for the next release.