Stylistic changes to activity.py and acentric.py to address flake8 & pylint issues

[apthorpe]

I took the liberty of running bits of thermo past pylint and flake8 to clean up semi-cosmetic issues. Specifically, I adjusted spacing, broke up long strings, and added comments, docstrings, and pylint directives to activity.py & acentric.py.

Before I got too carried away, I wanted to send the files over for review to see if the core project team would like me to continue flossing the code. I've tested both files and they don't throw any new or different errors than the originals (pylint can't find bubble_at_P in activity.py)

Please let me know if you have any questions or comments -- thanks!

[apthorpe]

BTW, if you're wondering why a random person would volunteer to style-check your code, here's the explanation: This sort of tedious cleanup work forces me to read the code and understand the thermo project's architecture, style, and conventions. I have built a pure Python library to implement DIPPR 801 property correlations (proprietary coefficients not included...) and it makes more sense for me to contribute the library to thermo rather than to release it as a separate module of dubious compatibility. This sort of cleanup serves as 'active lurking' to help me structure my code to work with the existing project. And regardless, reading code is good practice.

[coveralls]

Coverage decreased (-0.001%) to 92.668% when pulling 383a53d on apthorpe:master into c771f2b on CalebBell:master.

[CalebBell]

Hello Bob,

Thank you for the contribution! It's always exciting to get support of any kind. I would be very happy to work with you to integrate what code you've written regarding the DIPPR methods. Do you just mean their numbered correlations, or property predictions methods? I have quite a few of both, but it's not exhaustive.

It's never fun to receive criticism, but there are some things I'd like changed with this PR. There are also things I like quite a lot in it. Here are all my comments, positive and negative.

• pyflakes suggests module imports should be before all, but pep8 disagrees (https://www.python.org/dev/peps/pep-0008/#module-level-dunder-names) - I changed to pep8 a while back but used to do it the other way. I'm not sure why they disagree - do you know?
• The pylint directives at the top of files should be turned into a pylintrc for the project but some IDEs (including Spyder IIRC) don't use it unfortunately. There would be too much duplication putting it in each file.
• I'm not a fan of using \ to break lines - I prefer to enclose the statement in brackets, mostly because I've forgotten the \ at the end of the line before and then the next statement gets executed but not as part of the previous expression and it's not assigned to anything. Not a deal breaker on the PR though. As an example, forgetting the second \ silently breaks the following:

omega = log(101325.0/Pc) - 5.92714 + 6.09648/T_br + 1.28862*log(T_br) - \ 
                     0.169347*T_br**6)/(15.2518 - 15.6875/T_br - 13.4721*log(T_br) +
		      0.43577*T_br**6

• I developed an "internal" standard of having spaces between + and -, and not between / or *. I like your style of adding spaces where the groups between the signs are long such as indexing however.
• ''' Internal function - TBD ''' is not helpful; I'd go for "Helper to generate a list of strings representing the available methods for the function" or something similar, if I were to document them. Otherwise it's hopefully self-explanatory and more importantly not user facing so not really requiring documentation.
• I really like the string concatenation you used on the error message of one of the "Exception" statements; Definitely something I'll probably adopt throughout my projects eventually and use in new functions for now.
• If you search for an import and it's not showing in the file, it's not used (unless the tests break, which I don't think will happen anywhere). Hard to keep track of without pyflakes turned on. I find it too much of a distraction while coding though.
• All of the examples in the code are actually doctests, which get run automatically when py.test gets the argument "--doctest-modules". Not all of them are currently passing (in Chemical and eos/eos_mix only) so the those tests aren't being run automatically right now. Each of the failures need to be investigated and occurred when I was moving faster than I should have. But, the ones you've changed by making the output take multiple lines will all fail because doctests inconveniently need to take. The inputs to the doctests don't have to be on one line but the next line continuing them needs to start with "... ".
• I hadn't been saving the project's files with the setting for spaces at end of lines to be removed; would you mind adding that setting to your PR? While the history is being broken for style anyway, what might as well be part of the push.
• The analytical solutions in the Rachford-Rice function were generated by SymPy. I appreciate you splitting them up; the reason I didn't initially though was that the easiest way to check it was correct was to re-generate the code. Not an issue to add line breaks now because it's working and is checked against the numerical solution in the tests too.

To recap, for this to be mergable, I'm asking for:

• Fixed doctests
• Go ahead and remove the unused imports
• I'd rather no documentation than "TBD"; "Undocumented - use at your own risk; subject to removal without notice" would work too. The functions in activity are mostly exposed to users through the Mixture class.
• Add a .pylintrc instead of the declarations
• Revert the import/__all__ switch or figure out why one is better than the other
• Remove spaces at the ends of each file with your IDE (don't worry about it if yours doesn't support it)

If you have specific questions about the code base, I can definitely answer them too. The core team is unfortunately just me at this point. The style changes are definitely appreciated. I'm quite interested and curious about what you've built but would appreciate more style changes if you feel like it. Feedback on the Chemical and Mixture APIs or property prediction methods would be appreciated too.

Thanks again for the interest in the project,
Caleb

[apthorpe]

To be honest, I'd rather have a 'house style' coding standard than take the defaults from pylint, etc because it means you've actually thought about what you want and why. So no worries at all about feedback; I'll stick with your preferred style & revert the import/__all__ ordering (one of the tools complains if it sees imports after an assignment)

Some of the pylint directives should go into a config file; initially I just put them in the source so it's clear what rulesets are being ignored. That gives people the chance to ask whether it's good practice to mask over a particular style warning.

The TBD on documentation is meant to be visually irritating as a goad to replace it. I'll see what I can document myself and convert the remainder into TODOs so they are tracked but don't appear in the documentation.

I'll send more comments later; thanks for the quick & detailed feedback. FWIW, my day job is writing nuclear safety analysis code and despite not having much of a chemistry/ChE background, I do a lot of work with property data & correlations. The DIPPR module I have under development defines the numbered correlations but uses function pointers and lambdas to fix coefficients for a specific property. I'm not sure about the efficiency, but there's some protection against inadvertently changing correlation coefficients since they're effectively read-only once the lambda is assigned. It doesn't handle the odd cases where there are multiple sets of coefficients for different temperature ranges for a given property but it does expect to get high and low test pairs for validation which are part of the DIPPR 801 database.

Finally, thanks for making this code available - it's an impressive & important piece of work!

More later,

-- Bob

[apthorpe]

Fought with git all morning to upload revisions to github. Closing this pull request and starting a new one to pull from apthorpe:linting instead of apthorpe:master.