Change input file's behavior of loading families through kineticFamilies field.
#195
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| rmg.kineticsFamilies = kineticsFamilies | ||
| pass | ||
| elif kineticsFamilies == 'none': | ||
| rmg.kineticsFamilies = kineticsFamilies |
There was a problem hiding this comment.
Looks here like the 'none' behavior is the same as the 'all' behavior, but I see now that kineticsFamilies contains a different value.
There was a problem hiding this comment.
Technically could condense to the first 3 if/elif statements into 1.
|
I like your suggestions: |
Now the following options for kineticsFamilies in the input file are allowed: - When kineticsFamilies field is set to 'default', the RMG recommended families are loaded from the database. This is also selected when the kineticsFamilies field is completely missing - 'all' indicates all families are turned on. - 'none' means none of the families are turned on. This is good for debugging. - A list of families means only those are selected, e.g. ['H_Abstraction','R_Recombination']. You can also deselect all but certain families by using the ! flag, e.g. ['!Intra_Disproportionation','Substitution_O'] Added additional error raising in rmgpy.data.kinetics in case any of the if/elif statements fall through when code changes occur.
|
I think now it has the behavior we actually want, which is:
|
…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.
|
This looks good. But then I started tweaking it, and got carried away. Ended up sending you a pull request (onto this pull request) - connie#1 |
…base. Not just the ones listed as 'True.'
…ies. An exercise that showed me how bad I am at making unit tests quickly.
|
Excellent. Good work with the unit test! And Travis says it's safe to merge. Will do so now. |
rwest
added a commit
that referenced
this pull request
Apr 3, 2014
Change input file's behavior of loading families through `kineticFamilies` field, and various fixes. There's a lot more error checking now, and the error checking is tested in a unit test!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Please look at commit msg to observe current behavior.
Some issues I have with this current implementation:
Setting kineticFamilies to 'default' should probably have use a set of recommended families instead of loading all of them- we have those 'recommended' families flags but they aren't really used. This way we can turn certain families off for most people.
Setting kineticsFamilies to an empty list should probably give some error, instead of making it load all the families, we should probably force people to use the 'all' or 'none' or 'default' flags.
I don't like how omitting the kineticsFamilies entirely leads to setting it to 'default.' I think we should create an assertion any time the kineticsFamilies is not set to anything in the input file. Again, people should use the 'all','none', or 'default flags.'
Any thoughts?