Closed apthorpe closed 7 years ago
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.
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.
\
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
To recap, for this to be mergable, I'm asking for:
__all__
switch or figure out why one is better than the otherIf 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
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
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.
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!