Add BM property and use defaultdict for _BM attribute

[yalinli2]

This may not be the best practice for BioSTEAM, but this is what I'm doing for QSDsan since a lot of the equipment in the sanitation systems does not have BM, so I find it easier to default them to 1. Feel free to drop it - though I think it'll be good to make the BM dict public. Thanks!

from collections import defaultdict

some code some code some code...

    @property
    def BM(self):
        '''[dict] Bare module factors for all cost items, unspecified items will be set to one.'''
        return self._BM
    @BM.setter
    def BM(self, i:dict):
        if not isinstance(i, dict):
            raise TypeError('BM must be a dict, not {type(i).__name__}.')
        self._BM.update(i)

    @property
    def installed_cost(self):
        '''Total installed cost of the unit.'''
        BM = defaultdict(lambda:1)
        BM.update(self._BM)
        installed_cost = sum(self.installed_costs.values())

        return sum([i.installed_cost for i in self.auxiliary_units],
                   installed_cost)

    @property
    def installed_costs(self):
        '''[dict] Installed cost of each equipment.'''
        BM = defaultdict(lambda:1)
        BM.update(self._BM)
        installed_costs = {i: BM[i]*j for i,j in self.purchase_costs.items()}

        for i in self.auxiliary_unit_names:
            installed_costs[i] = getattr(self, i).installed_cost
        return installed_costs

[yoelcortes]

Hi Yalin,

Thanks for sharing this. It may be helpful for some users if they understand how it works. However, there are a couple of drawbacks to this implementation that may not be apparent:

• Changing a BM value through the object property changes it for all objects of the same class and subclasses. For example, let's say D1 is a distillation column, then "D1.BM['Column'] = 2" would change the BM for all distillation columns (including subclasses like Shortcut columns and Binary distillation columns). This may not be expected.
• The public property may only be accessed by the instance, not the class, yet _BM is a class attribute.
• Setting a property as an object, yet the property has no connection to that object. The following test would fail:

  U1.BM = dct = {'A': 1}
  dct['B'] = 2
  assert U1.BM['B'] == 2

• Regarding the default-dict, spelling mistakes and missing BM values would go unnoticed:

  class X(Unit):
    _BM = {'vessel': 4.3, 'pumb': 2.0}
    def _cost(self):
        self.purchase_costs['vesel'] = 1e3 # Misspelled
        self.purchase_costs['pump'] = 1.5e3 # Misspelled in '_BM'
        self.purchase_costs['platform'] = 2e3 # Missing

• Creating a new default dict each time the property is accessed will add some overhead. Defaultdicts are also slower than native python dictionaries. Far from a bottleneck for biosteam overall, but still significant when looking at just the property.

Most of these drawbacks could potentially lead to bad estimations of capital costs that may go unnoticed. One possible solution to setting BM for a given instance of a unit operation could be to use "getter" and "setter" methods instead of properties, which I'm open to.

Thanks!

[yalinli2]

Hi Yoel, thanks for the response, the first point is a good argument! I didn't expect this. So definitely not setting _BM.

For the "getter" and "setter" methods, can you give me an example or point me to the documentation? I didn't quite get it and most of the posts I saw were about using property. Thanks!

[yoelcortes]

Sure! Python properties work as getter and setters, but methods are preferred for some applications where the object retrieved is not the same as the object setted. Here is an example:

def get_BM(self):
    return self._BM.copy() # Do not allow user to change BM; only set_BM is allowed

def set_BM(self, BM):
    if '_BM' not in self.__dict__: self._BM = self._BM.copy() # Prevent changing BM of all classes
    for i in BM: # Prevent spelling errors
        if i not in self._BM: raise ValueError(f"purchase item '{i}' does not exist")
    self._BM.update(BM)

Hope this helps, Thanks!

[yalinli2]

Oh I got what you meant, I thought getter and setter were dunder methods. Then why not just

    @property
    def BM(self):
        '''[dict] Bare module factors for all cost items, unspecified items will be set to one.'''
        if '_BM' not in self.__dict__.keys():
            self._BM = self._BM.copy()
        return self._BM
    @BM.setter
    def BM(self, i:dict):
        if '_BM' not in self.__dict__.keys():
            self._BM = self._BM.copy()
        self._BM.update(i)

This is more natural than having the set_BM and get_BM. Note that we need to check if the class instance has _BM in both getter and setter because we don't know which one the user will access first.

Alternatively, there's the fancy way of using descriptors:

class BareModule:
    '''For unit BM (bare module factors).'''

    def __get__(self, obj, objtype=None):
        if '_BM' not in obj.__dict__:
            _BM = defaultdict(lambda:1)
            _BM.update(obj._BM)
            obj._BM = _BM

        # If don't want defaults
        # if '_BM' not in obj.__dict__:
        #     obj._BM = obj._BM.copy()

        return obj._BM

    def __set__(self, obj, BM:dict):
        if '_BM' not in obj.__dict__:
            _BM = defaultdict(lambda:1)
            _BM.update(obj._BM)
            obj._BM = _BM

        # If don't want defaults
        # if '_BM' not in obj.__dict__:
        #     obj._BM = obj._BM.copy()

        obj._BM.update(BM)

Then when defining the Unit, add BM = BareModule()

An example:

import qsdsan as qs

H2O = qs.Component('H2O', search_ID='H2O', particle_size='s',
                   degradability='s', organic=False)
qs.set_thermo(qs.Components((H2O,)))

ss1 = qs.SanStream(H2O=100)
ss2 = qs.SanStream(H2O=100)

M1 = qs.sanunits.MixTank('M1', ins=ss1)
M1.BM['Tanks'] = 1
M1.simulate()
print(M1.BM['Tanks']) # -> 1
print(type(M1.BM)) # -> <class 'collections.defaultdict'>
print(M1.installed_costs) # -> {'Tanks': 4386.336513753271}

M2 = qs.sanunits.MixTank('M2', ins=ss2)
M2.simulate()
M2.results()
print(M2.BM['Tanks']) # -> 2.3
print(type(M2.BM)) # -> <class 'collections.defaultdict'>
print(M2.installed_costs) # {'Tanks': 10088.573981632522}

I agree with you that using defaultdict certainly lose some checks and not using them in BioSTEAM, but I may still use it in QSDsan considering how QSDsan will be used, thanks!

[yoelcortes]

Hi Yalin,

The example getter and setter methods for BM I gave is really not a good one. I'm actually not too happy with the setter... perhaps and "override_BM" method would be more appropriate... I'd need to think about it more.

Properties are really nice to remove coding hassles and simplify code, but there are a couple drawbacks in this case. Using python properties gives the impression that the property is like an attribute, but with a checks and balances. This is not the case for the suggested implementation. For example, the following tests would fail:

U1.BM = {'B': 1, 'C': 2}
U1.BM = BM = {'A':  1}
assert U1.BM is BM # Fails
assert U1.BM == {'A': 1} # Fails too!

Additionally, if the user retrieves BM, then it's no longer a class attribute and changes to the class attribute _BM will not be reflected in the instance. This could become a problem with uncertainty analysis on the bare module factor if not clearly stated. For example:

# Let's say U1._BM['A'] is 1.
BM = U1.BM
U1._BM['A'] = 2
assert BM['A'] == 2 # Fails

Although this last example would also affect an "override_BM" setter method, we could add a warning in the method. Note that method documentations are more likely to be read than property documentations (in some IDEs, they appear when accessing the method from the instance; property documentation only appears for the class)

Quoting from the zen of python, "explicit is better than implicit" and such a property implementation performs rather implicit things.

Just my thoughts on the matter, but there are always pros and cons Thanks,

[yalinli2]

Hi Yoel, thanks for thinking over this! True on the uncertainty analysis point... I'll close this issue for now, let me know if you think of a good way to deal with it, thanks!!!