Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add function to calculate pressure drop using Ergun equation #18

Merged
merged 3 commits into from
Apr 21, 2021

Conversation

jAniceto
Copy link
Contributor

  • Added a function to calculate pressure drop using the Ergun equation
  • Added corresponding documentation
  • Added corresponding tests

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

@wigging
Copy link
Owner

wigging commented Apr 20, 2021

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

@wigging
Copy link
Owner

wigging commented Apr 20, 2021

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
Copy link
Contributor Author

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
Copy link
Owner

wigging commented Apr 20, 2021

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
Copy link
Owner

wigging commented Apr 20, 2021

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
Copy link
Contributor Author

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 wigging changed the base branch from main to develop April 21, 2021 03:14
@wigging
Copy link
Owner

wigging commented Apr 21, 2021

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.

@wigging wigging merged commit 65e2fe5 into wigging:develop Apr 21, 2021
@jAniceto jAniceto deleted the pressure-drop branch April 21, 2021 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants