duartegroup / autodE

automated reaction profile generation
https://duartegroup.github.io/autodE/
MIT License
166 stars 51 forks source link

Autouse `temporary_config` in tests #227

Closed t-young31 closed 1 year ago

t-young31 commented 1 year ago

Currently tests can modify the shared global Config class. They should be rewirtten to use the new temporary_config context manager

shoubhikraj commented 1 year ago

@t-young31 I would just like to add here that even with temporary_config context manager, the shared global Config is still modified. The context manager just stores the state of Config when it is called, and restores it with Config.__dict__.update() when it exits.

So things like this will happen:

import autode as ade
from autode import Config

kwds = Config.ORCA.keywords  # gets a reference to Config.ORCA.keywords object

with ade.temporary_config():  # saves Config.__dict__
  kwds.sp.functional = 'B3LYP'  # changes the global config state
  # --calculation--
# when context manager returns, Config.__dict__.update() is called
# which restores the state of previous Config, but that also means
# all values in Config are replaced
assert str(kwds.sp.functional) == 'B3LYP'  # this is True
# because kwds now refers to the old Config.ORCA.keywords object
# and changing kwds no longer changes anything in new Config

So for any mutable types like list or object etc. previous references to them will stop working. For immutable types, taking reference is useless.

t-young31 commented 1 year ago

thanks @shoubhikraj – an excellent point. I'm ok with that behaviour/quirk if you are?

shoubhikraj commented 1 year ago

@t-young31 Yes, I think this behaviour is ok. Fixing it is possible, but would require knowing which item is mutable object, and which is not, and there would be many layers of nested mutable objects - Config, ORCA, keywords, sp. I don't think it would be easy to handle all such cases. Also python does not provide any good way of identifying mutable vs immutable objects.

I would be ok with leaving the behaviour, but I think it might be worth adding an extra page in the examples to show the quirks of Config. Is that fine with you?