Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initialize the global RMGDatabase object upon calling it #1565

Merged
merged 1 commit into from
Mar 22, 2019
Merged

Conversation

alongd
Copy link
Member

@alongd alongd commented Mar 21, 2019

Initialize the global RMGDatabase object upon calling it even (and especially) if it already exists.
This solves cases when using RMG's API where the database doesn't get appropriately updated.

@alongd alongd self-assigned this Mar 21, 2019
@alongd alongd requested review from mjohnson541 and mliu49 March 21, 2019 18:06
@codecov
Copy link

codecov bot commented Mar 21, 2019

Codecov Report

Merging #1565 into master will increase coverage by 0.01%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1565      +/-   ##
==========================================
+ Coverage   41.84%   41.86%   +0.01%     
==========================================
  Files         165      165              
  Lines       28008    28007       -1     
  Branches     5713     5713              
==========================================
+ Hits        11721    11726       +5     
+ Misses      15498    15493       -5     
+ Partials      789      788       -1
Impacted Files Coverage Δ
rmgpy/data/rmg.py 71.54% <50%> (+0.57%) ⬆️
rmgpy/data/kinetics/family.py 58.56% <0%> (+0.29%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c39c888...5e907df. Read the comment docs.

@mliu49
Copy link
Contributor

mliu49 commented Mar 21, 2019

I think this looks good. Do we want the re-initialization message to be debug or warning? Personally, I'm leaning more towards warning since I think users should be aware that it is happening.

@alongd
Copy link
Member Author

alongd commented Mar 21, 2019

done!

@mjohnson541
Copy link
Contributor

Looks good to me!

logging.warning("Should only make one instance of RMGDatabase because it's stored as a module-level variable!")
logging.warning("Unexpected behaviour may result!")
if database is not None:
logging.warn('An instance of RMGDatabase already exists. Re-initializing it.')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should use logging.warning. logging.warn is just a pointer to logging.warning, and is now deprecated in Python 3.

even (and especially) if it already exists.
This solves cases when using RMG's API where the database doesn't get
appropriately updated.
@alongd
Copy link
Member Author

alongd commented Mar 21, 2019

good to know, done

@alongd
Copy link
Member Author

alongd commented Mar 22, 2019

I can approve that the RMG/ARC feature works as expected when cherry-picking this commit.
In fact, and strangely, the implementation of resetting the database after each call to RMG or ARC via something like

def clear_rmg_database():
    """Set the RMG database object instance to None"""
    rmg_db = getDB()
    rmg_db.thermo = None
    rmg_db.transport = None
    rmg_db.forbiddenStructures = None
    rmg_db.kinetics = None
    rmg_db.statmech = None
    rmg_db.solvation = None

was problematic, and in the second iteration RMG comlained about missing components in the database (e.g., can't check forbidden structures since database.forbiddenStructures was None; when I manually initialized it with the correct object, it complained that a different part was None). I thought I did use this function in an order that makes sense.

In any case, I'm very happy with the implementation in this PR.

Copy link
Contributor

@mliu49 mliu49 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I'll go ahead and merge.

@mliu49 mliu49 merged commit 83ad0f2 into master Mar 22, 2019
@mliu49 mliu49 deleted the database branch March 22, 2019 16:55
@alongd alongd mentioned this pull request Mar 25, 2019
@mliu49 mliu49 mentioned this pull request May 15, 2019
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants