search

mcgibbon / sympl

A toolkit for building planetary/Earth system models in Python
http://sympl.readthedocs.io
Other
48 stars 14 forks source link

Possible bug in TendencyStepper #37

Open JoyMonteiro opened 6 years ago

JoyMonteiro commented 6 years ago

The gray longwave component in climt had a tendency which did not define its dims. Since this property is available in the input_properties of the component, one would imagine that this is ok. However, since the code for output_properties in TendencyStepper does not pass the input properties to combine_component_properties, we get an InvalidPropertyDictError.

This is in the line https://github.com/mcgibbon/sympl/blob/3277b7bd776a5841e9667c178c357548fdd5f344/sympl/_core/tendencystepper.py#L84

This occurs when I try to create a timestepper with gray gas radiation alone.

I have fixed the issue by adding a dims property to the component. However, I wanted to check if this is expected behaviour.

mcgibbon commented 6 years ago

Could you write a MCVE for me to reproduce the error?

JoyMonteiro commented 6 years ago

I have fixed the component, but if you comment out the following line:

https://github.com/CliMT/climt/blob/c4a340d225b8b48790f012764527ccb1735827fe/climt/_components/radiation.py#L54

in radiation.py and run the following line:

AdamsBashforth([GrayLongwaveRadiation])

I get an error.

mcgibbon commented 6 years ago

This is not a proper fix, because there is still an underlying bug in TendencyStepper. You shouldn't need to specify dims on a tendency when that quantity is present in the inputs.

JoyMonteiro commented 6 years ago

Yes, which is why i brought it to your attention.