WIP: Cleanup chemkin module#215
Closed
wking wants to merge 2 commits intoReactionMechanismGenerator:masterfrom
Closed
WIP: Cleanup chemkin module#215wking wants to merge 2 commits intoReactionMechanismGenerator:masterfrom
wking wants to merge 2 commits intoReactionMechanismGenerator:masterfrom
Conversation
…space From PEP 8 [1]: Wildcard imports (from <module> import *) should be avoided, as they make it unclear which names are present in the namespace, confusing both readers and many automated tools. There is one defensible use case for a wildcard import, which is to republish an internal interface as part of a public API (for example, overwriting a pure Python implementation of an interface with the definitions from an optional accelerator module and exactly which definitions will be overwritten isn't known in advance). I use a leading underscore in the name to avoid conflicts with local 'kinetics' variables, and because the imported module is a private member of chemkin. We don't want folks using: from rmgpy.chemkin import kinetics [1]: http://legacy.python.org/dev/peps/pep-0008/#imports
Split it into _readKineticsReaction (for the first line) and _readKineticsLine (for subsequent lines). This makes reading readKineticsEntry easier by limiting the scope of local variables and allowing you to focus on the higher-level logic without line-level distraction. I left as many lines unchanged as possible, excepting some reworking to handle state during the _readKineticsLine loop. Instead of maintaining a handful of per-model variables and using 'None' to detect which have been set, I've used a single 'kinetics' dict and use key presence to detect which have been set. The allows subsequent lines to clobber kinetics entries from previous lines (e.g. shifting 'arrhenius high' to 'arrhenius low') without having to pass around a bunch of None variables.
Contributor
Author
Member
|
Hard to blame you for a few bugs: coverage reports that our unit tests check a whopping 3% of that code! :) |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
I'm pushing this as a heads-up that I'm churning this section of code
(let me know if someone else is working here, and I'll wait until
they're done). Don't merge it yet ;).
While reviewing #213 I noticed a wildcard import (from … import *),
and thought I'd take a moment to clean it up. However, my first pass
at that (through chemkin.py) turned up a number of large, complicated
functions that could use some refactoring. This series is my attempt
to polish up the Python in chemkin without breaking any of the logic
that I don't particularly understand ;). I'll push periodically so
Travis can see if I broke anything, while I try to split this logic
into easily-digestible bites.