Skip to content

Suggestions for your universalDatabase pull request 195#1

Closed
rwest wants to merge 5 commits into
connie:universalDatabasefrom
rwest:195
Closed

Suggestions for your universalDatabase pull request 195#1
rwest wants to merge 5 commits into
connie:universalDatabasefrom
rwest:195

Conversation

@rwest

@rwest rwest commented Apr 3, 2014

Copy link
Copy Markdown

I got carried away with suggestions, and ended up changing so much that my code review needs reviewing itself! sigh

rwest added 5 commits April 2, 2014 22:30
…t file.

Less code, hopefully easier to understand.
With this commit, the recommended.py file in 
RMG-database/input/kinetics/families/recommended.py
should just contain a dictionary, not a function call

recommendedFamilies = {
   '1,2-Birad_to_alkene': True,
   '1,2_Insertion': False,
  }

I'll make a corresponding commit in the RMG-database project.

Other changes:
Reduced code duplication, tidied up comments, and added more 
sanity checks on input, eg. we check that the list is all 
true or all false, not a mix, and we check that all the requested
families can be found in the database (before we just silently skipped them)
and check that all families in the database are listed in the recommended.py 
file as either True or False (in case we forget to add them).
This is a small change, but it changes the indentation of 
a huge block of code so looks like a big one.
The loading of the recommendation list has been moved to its own method
because it was getting complicated, and we want to be able to store this
more permanently, even if we aren't going to use it. This will allow us
to interrogate it, and, for example, to export the Java-style database with it.
(is this still going to be neccessarry?)
Now that we have this data to hand, we may as well preserve it
and pass it along.
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.

1 participant