ajwdewit / pcse

Repository for the Python Crop Simulation Environment
Other
191 stars 127 forks source link

Fix copying of CABOFileReader, PCSEFileReader, DummySoilDataProvider #84

Closed burggraaff closed 6 months ago

burggraaff commented 6 months ago

Description

CABOFileReader, PCSEFileReader, and DummySoilDataProvider are subclassed from dict and therefore inherit its dict.copy method. This causes an issue when the user tries to use this method to copy an instance of these objects, because dict.copy always returns a dict object, not the subclass.

The fix has been implemented for these three classes because they are user-facing (by which I mean I have used them). There are other subclasses of dict, like MultiCropDataProvider, where I'm not sure if it's necessary; and others, like the various CGMS DataProviders, which I have not used at all and thus don't want to blindly edit. A more general fix might be to switch from subclassing dict to subclassing collections.UserDict, but this is not trivial. Additionally, a similar issue exists for classes that subclass list or set, such as YAMLAgroManagementReader, but again I am not sure if these are commonly copied by users and/or I don't have experience working with these classes. However, it should be very straight-forward to apply the same fix elsewhere. Finally, other classes do not come with an inherited-but-broken .copy method and thus do not require any changes.

Example

A parameter file 'parfile.cab' which looks like this:

** CROP DATA FILE for use with WOFOST Version 5.4, June 1992
**
** WHEAT, WINTER 102
** Regions: Ireland, central and southern UK (R72-R79),
**          Netherlands (not R47), northern Germany (R11-R14)
CRPNAM='Winter wheat 102, Ireland, N-U.K., Netherlands, N-Germany'
CROP_NO=99
TBASEM   = -10.0    ! lower threshold temp. for emergence [cel]
DTSMTB   =   0.00,    0.00,     ! daily increase in temp. sum
            30.00,   30.00,     ! as function of av. temp. [cel; cel d]
            45.00,   30.00
** maximum and minimum concentrations of N, P, and K
** in storage organs        in vegetative organs [kg kg-1]
NMINSO   =   0.0110 ;       NMINVE   =   0.0030

Can be read as follows:

>>> from pcse.fileinput import CABOFileReader
>>> cabo = CABOFileReader("parfile.cab")

>>> print(cabo)
** CROP DATA FILE for use with WOFOST Version 5.4, June 1992
**
** WHEAT, WINTER 102
** Regions: Ireland, central and southern UK (R72-R79),
**          Netherlands (not R47), northern Germany (R11-R14)
------------------------------------
CROP_NO: 99 <class 'int'>
TBASEM: -10.0 <class 'float'>
NMINSO: 0.011 <class 'float'>
NMINVE: 0.003 <class 'float'>
CRPNAM: Winter wheat 102, Ireland, N-U.K., Netherlands, N-Germany <class 'str'>
DTSMTB: [0.0, 0.0, 30.0, 30.0, 45.0, 30.0] <class 'list'>

>>> type(cabo)
pcse.fileinput.cabo_reader.CABOFileReader

Current behaviour

The resulting cabo instance has a .copy method inherited from dict, which a user may encounter and try to use (I certainly did). However, this leads to unexpected and unwanted behaviour:

>>> cabo_copy = cabo.copy()
>>> print(cabo_copy)
{'CROP_NO': 99, 'TBASEM': -10.0, 'NMINSO': 0.011, 'NMINVE': 0.003, 'CRPNAM': 'Winter wheat 102, Ireland, N-U.K., Netherlands, N-Germany', 'DTSMTB': [0.0, 0.0, 30.0, 30.0, 45.0, 30.0]}

>>> type(cabo_copy)
dict

>>> print(cabo_copy.header)
Traceback (most recent call last):

  Cell In[8], line 1
    print(cabo_copy.header)

AttributeError: 'dict' object has no attribute 'header'

Solution

A .copy method has been added which uses the Python standard library copy.copy shallow-copying functionality. This makes cabo.copy() equivalent to copy.copy(cabo), which works properly out of the box.

>>> cabo_copy = cabo.copy()
>>> print(cabo_copy)
** CROP DATA FILE for use with WOFOST Version 5.4, June 1992
**
** WHEAT, WINTER 102
** Regions: Ireland, central and southern UK (R72-R79),
**          Netherlands (not R47), northern Germany (R11-R14)
------------------------------------
CROP_NO: 99 <class 'int'>
TBASEM: -10.0 <class 'float'>
NMINSO: 0.011 <class 'float'>
NMINVE: 0.003 <class 'float'>
CRPNAM: Winter wheat 102, Ireland, N-U.K., Netherlands, N-Germany <class 'str'>
DTSMTB: [0.0, 0.0, 30.0, 30.0, 45.0, 30.0] <class 'list'>

>>> type(cabo_copy)
pcse.fileinput.cabo_reader.CABOFileReader

>>> cabo_copy == cabo
True

>>> cabo_copy is cabo
False
ajwdewit commented 6 months ago

Dear burggraaf, thank for the fix. I never used dict.copy(), but instead copy.copy(). But it is indeed better this way. Allard