ReactionMechanismGenerator / ARC

ARC - Automatic Rate Calculator
https://reactionmechanismgenerator.github.io/ARC/index.html
MIT License
42 stars 21 forks source link

rmgdb object does not load families if they are already loaded #702

Closed kfir4444 closed 9 months ago

kfir4444 commented 9 months ago

Describe the bug If RMGDB object has .kinetics.families, it will not load more families, even when the user requested to load a different set of reaction families.

How to reproduce

>>> import arc.rmgdb as rmgdb

>>> db = rmgdb.make_rmg_database_object()
>>> rmgdb.load_families_only(db, ['1+2_Cycloaddition'])
>>> print(db.kinetics.families)
    {'1+2_Cycloaddition': <ReactionFamily "1+2_Cycloaddition">}
>>> rmgdb.load_families_only(db, ["1,2-Birad_to_alkene"])
>>> print(db.kinetics.families)
    {'1+2_Cycloaddition': <ReactionFamily "1+2_Cycloaddition">}

Expected behavior To load reaction families as the user requested.

kfir4444 commented 9 months ago

I have done some experiments using the timeit package, and loading the default reaction families takes ~6.73 seconds (averaged over 10 repeats), versus loading all reaction families, which takes ~7.26 seconds. Since an expected difference of 0.53 seconds is not a major setback when compared with the other calculations, I suggest to avoid fixing this issue, and always load all reaction families, unless the user specifically asks not to.

Script used for timing:

import timeit

setup = '''
from arc.rmgdb import load_families_only, determine_family, make_rmg_database_object
import arc.rmgdb

del arc.rmgdb.rmg_database_instance # cleaning the global variable
arc.rmgdb.rmg_database_instance = None # avoiding NameErrors
'''

test_1 = '''
db = make_rmg_database_object()
load_families_only(db)
'''

test_2 = '''
db = make_rmg_database_object()
load_families_only(db, "all")
'''

times_regular_load = timeit.repeat(setup=setup,
                                   stmt=test_1,
                                   repeat=10,)

times_all_load = timeit.repeat(setup=setup,
                                   stmt=test_2,
                                   repeat=10,)
kfir4444 commented 9 months ago

This issue is not expected to be encountered by in a normal ARC use, and is circumvented by loading all reaction families, as in #704