MetPy icon indicating copy to clipboard operation
MetPy copied to clipboard

Helpful declarative `__setattr__` and trait validation

Open dcamron opened this issue 3 years ago • 8 comments

As it was ripped out of #1903 for now, it might be helpful to users to

  • [ ] Expand validation and type-checking where applicable for declarative traits
  • [ ] Craft a non-intrusive MetPyHasTraits.__setattr__ to provide errors for users attempting to set traits that don't exist (e.g. misspellings)
  • [ ] Document/exemplify these and additions from #1907, #1903

dcamron avatar Aug 09 '21 22:08 dcamron

Does this issue address the problem described in this notebook?

https://gist.github.com/sgdecker/8c90f2300da03de296e1d934b47e1cd1

sgdecker avatar Aug 12 '21 14:08 sgdecker

Does this issue address the problem described in this notebook?

https://gist.github.com/sgdecker/8c90f2300da03de296e1d934b47e1cd1

Accomplishing these tasks should ideally a.) tell you that 'COUNTRIES' is invalid, and b.) tell you so when you attempt to set the trait as opposed to when the figure is drawn. So, "partial yes".

I don't know that this will address the plot parent issue you're having in that notebook. In the mean time it does look like re-creating the PlotObs in the cell above (re-run cells 4-5 instead of just 5) can fix this for now, a full restart/run all not necessary. I'll see if I can boil down where this is happening and open a new issue for it, unless you already have a small reproducible code snippet and want to do that yourself!

dcamron avatar Aug 12 '21 17:08 dcamron

That notebook is the smallest snippet I have, so by all means, go ahead and open a new issue. Thanks!

sgdecker avatar Aug 12 '21 18:08 sgdecker

Here's a minimal reproducer:

from datetime import datetime
from metpy.cbook import get_test_data
from metpy.io import parse_metar_file
from metpy.plots.declarative import PlotObs, MapPanel, PanelContainer

sfc_obs = parse_metar_file(get_test_data('2020010600_sao.wmo', False))

obs = PlotObs()
obs.data = sfc_obs
obs.time = datetime.utcnow()

panel = MapPanel()
panel.projection = 'lcc'
panel.layers = ['countries','states']
panel.plots = [obs]

try:
    pc = PanelContainer()
    pc.panels = [panel]
    pc.show()
except Exception:
    pass

panel = MapPanel()
panel.projection = 'lcc'
panel.plots = [obs]

The call to show() is critical to getting the failure. I'm sure it's some kind of imperfect state management going on here...

dopplershift avatar Aug 16 '21 18:08 dopplershift

Let me know if this is worth a new issue, and I realize the existing inconsistencies are just holdovers from the underlying Cartopy feature names, but a list like panel.layers = ['coastline', 'borders', 'states'] is maddeningly inconsistent.

Why is coastline singular? borders is vague; why isn't it countries?

My request would be to add aliases ('coastlines' = 'coastline', 'countries' = 'borders', and perhaps others).

sgdecker avatar Aug 24 '21 14:08 sgdecker

@sgdecker Can you open that as a new feature request? It's reasonable to do for the declarative interface.

dopplershift avatar Aug 24 '21 19:08 dopplershift

Please let me know if this should be a separate issue, but a student was flummoxed when the following code excerpt:

pc = PanelContainer()
pc.panel = [panel1, panel, panel3, panel2]
pc.size = (15,12)
pc.show()

generated the error TraitError: The 'panels' trait of a PanelContainer instance contains an Instance of a List which expected a Panel, not the list [<metpy.plots.MapPanel object at 0x7fd1cace3520>, <metpy.plots.MapPanel object at 0x7fd1cad4cc10>, <metpy.plots.MapPanel object at 0x7fd1cabe8700>, <metpy.plots.MapPanel object at 0x7fd228abd340>].

While a savvy debugger can spot the problem pretty quickly, it isn't entirely obvious (especially when there are many lines of code before this). I believe the panel.setter in the PanelContainer class should check that val is indeed a singular panel.

sgdecker avatar Feb 09 '24 16:02 sgdecker

@sgdecker Yeah, we could probably just check that what's passed in passes isinstance(val, Panel) since that's just a helper property and not a Traitlet.

dopplershift avatar Feb 13 '24 20:02 dopplershift