Skip to content

Sg/add pr guidelines#13

Merged
skygering merged 7 commits intomasterfrom
sg/add_pr_guidelines
Jun 24, 2025
Merged

Sg/add pr guidelines#13
skygering merged 7 commits intomasterfrom
sg/add_pr_guidelines

Conversation

@skygering
Copy link
Copy Markdown
Contributor

Added another set PR template and again fixed the deprecated trapz function name.

@skygering
Copy link
Copy Markdown
Contributor Author

There are a few other test warnings I would like to fix. Within the momentum file (here and here), we are taking the square root of a negative number. This is throwing a warning and returning NaN. This is clearly expected behavior, because it is texted for within the tests. However, it isn't good practice to depend on this type of behavior (e.g. the return of a failed call). We should test if it is going to be a negative, and if so, throw a warning that is more physical (i.e. "in this scenario you can't calculate induction because") and manually set it to NaN.

I can do this, but what is the Cx that is passed into both functions?

@kirbyh
Copy link
Copy Markdown
Member

kirbyh commented Jun 22, 2025

I can do this, but what is the Cx that is passed into both functions?

The variable Cx is the axial force coefficient, which is equivalent to $C_T$ in momentum theory. It makes sense to me to check whether the value under the radical $-C_T^2 \sin^2\gamma - 16C_T + 16$ is negative and to mask with nans before continuing, and maybe to raise a more descriptive warning. This value (and also the simpler $1-C_T$ in classical momentum theory) will be negative at high thrust (in no yaw: $C_T > 1$), where the UMM or other empirical high-thrust corrections are needed to resolve the BEM solution. If there is a standard way to put this in a warning (and to be able to toggle the warnings on/off), that would be useful

@iupfal
Copy link
Copy Markdown
Contributor

iupfal commented Jun 23, 2025

Great idea, Skylar! Adding an informative warning and manually setting the NaN values in both cases makes sense to me.

Copy link
Copy Markdown

@lynna-deng lynna-deng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pull request requirements look sensible to me -- thank you for creating the template, Skylar! Noted also that tests relying on errors are not good practice.

@iupfal
Copy link
Copy Markdown
Contributor

iupfal commented Jun 23, 2025

The PR requirements look good, thanks for adding these!

I tested on a few different python versions. The package doesn't work for newer python versions so maybe we can add this to the pyproject.toml (python = ">=3.9,<3.12").

Summary of testing:
3.13 can't install
3.12 can't install
3.11 installs; passes tests
3.10 installs, passes tests
3.9 installs; passes tests

@skygering
Copy link
Copy Markdown
Contributor Author

I think in hindsight, I am going to move fixing the warnings to another PR, to be done at another time. It seems like their are complicating factors to fixing the trapz warning (i.e. depending on a library that Jamie wrote), and I would rather get the PR checklist in ASAP than fix the warnings at least for now.

However, I will add in the python version limiting.

@skygering
Copy link
Copy Markdown
Contributor Author

Additionally, I got it to run on Python 3.12 and 3.13, so I think any of those changes will also be made in a separate PR if we determine they're needed. This will just be the PR checklist @kirbyh @iupfal @lynna-deng

@skygering skygering merged commit 604ba5b into master Jun 24, 2025
8 checks passed
@skygering skygering deleted the sg/add_pr_guidelines branch June 24, 2025 16:42
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.

4 participants