search

wigging / chemics

A Python package for chemical engineering
https://chemics.readthedocs.io
MIT License
169 stars 15 forks source link

Add function to calculate pressure drop using Ergun equation #18

Closed jAniceto closed 3 years ago

jAniceto commented 3 years ago

This is a small pull request to test the git workflow.

wigging commented 3 years ago

Thanks. I'll pull down the changes and let you know if I have any questions.

wigging commented 3 years ago

I ran your code and encountered several issues:

  1. When building the documentation, the footnote in the function's docstring is not referenced. It should be [1]_ instead of [1].
  2. The Python code doesn't adhere to the Flake8 style. Which editor or IDE do you use for writing Python code? You should be able to install a plugin for your editor to enforce the coding style.
  3. The pressure drop page needs to be listed in the table of contents section for the documentation. See the toctree in the index.rst file.
jAniceto commented 3 years ago

Hi. Thanks for the feedback. Regarding your points:

  1. I was going to remove the numbering after noticing several other function had references with no numbering. Is that ok?
  2. I am using VSCode and I installed the flake8 plugin. The only problem I get from flake8 is in the .. math:: line in the function docstring. I saw several other function where the equations did not wrap so I though I was supposed to follow that style.
  3. Missed that. Will add.
wigging commented 3 years ago

Yes, you can remove the numbering. But if you want to use a footnote for a reference it needs to have an underscore such as [1]_ otherwise Sphinx will complain and not properly build the documentation.

wigging commented 3 years ago

For the Flake8 style, this

    pressure_drop = 150*mu*(1-epsilon)**2*u0/(dp**2*epsilon**3) + \
        1.75*rhof*(1-epsilon)*u0**2/(dp*epsilon**3)

should be

    pressure_drop = 150 * mu * (1 - epsilon)**2 * u0 / (dp**2 * epsilon**3) + \
        1.75 * rhof * (1 - epsilon) * u0**2 / (dp * epsilon**3)

I am ignoring the E501,W503,W605 errors which I should mention in the contribution document.

jAniceto commented 3 years ago

I'll have to check why my linter isn't signaling that then.

I've added commits to fix the reference, toctree, and flake8 issues. Think that covers the problems.

wigging commented 3 years ago

Thanks, everything looks good. I changed the base branch to develop instead of merging directly into the main branch. Your contributions will be part of version 21.5 which will be released in May. If you decide to submit more pull requests, please make them to the develop branch and not to the main branch.