Skip to content

FIX: Motors UnitTesting Warnings #422

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

Merged
merged 7 commits into from
Oct 7, 2023
Merged

Conversation

phmbressan
Copy link
Collaborator

Pull request type

  • Code changes (bugfix, features)
  • Code maintenance (refactoring, formatting, tests)
  • ReadMe, Docs and GitHub updates
  • Other (please describe):

Checklist

  • Tests for the changes have been added (if needed)
  • Docs have been reviewed and added / updated
  • Lint (black rocketpy/ tests/) has passed locally
  • All tests (pytest --runslow) have passed locally

Current behavior

The conventional pytest run raised some warnings regarding wrong burn_time input and integration precision not being achieved.

Furthermore, the way the SolidMotor grain regression ODE was written could be improved a bit both in speed and readability with better local variable naming/choice.

New behavior

The pytest warnings were solved and, locally, they do not show up anymore. In order to improve the SolidMotor grain regression ODE, the local variable scheme was changed and the simulation jacobian was added following scipy.solve_ivp recomendation for the LSODA solver.

Breaking change

  • Yes
  • No

@phmbressan phmbressan added Function Everything related to the Function class Tests Regarding Tests Motors Every propulsion related issue or PR labels Sep 30, 2023
@phmbressan phmbressan added this to the Release v1.X.0 milestone Sep 30, 2023
@phmbressan phmbressan self-assigned this Sep 30, 2023
@phmbressan phmbressan changed the title Enh/solid ode jacobian Fix Motors Testing Warnings Sep 30, 2023
@phmbressan phmbressan changed the title Fix Motors Testing Warnings FIX: Motors UnitTesting Warnings Sep 30, 2023
@@ -2229,7 +2229,7 @@ def integral(self, a, b, numerical=False):
ans = np.trapz(y_integration_data, x_integration_data)
else:
# Integrate numerically
ans, _ = integrate.quad(self, a, b, epsabs=0.001, limit=10000)
ans, _ = integrate.quad(self, a, b, epsabs=1e-4, epsrel=1e-3, limit=1000)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is, likely, the most polemic line of this PR. Therefore, I will explain the main points that motivated this change:

  • According to the scipy quad docs the tolerances follow: abs(i-result) <= max(epsabs, epsrel*abs(i));

  • The integration warning on the tests happened in the computation of the average of the exhaust velocity. The curve was fairly well behaved, but likely had some property that made the integrator overstimate the error to about 10 (while the result was about 50_000).

  • Then, the configs prior to this PR were epsabs=1e-3 and epsrel=1.49e-8. It is easy to see that the relative tolerance is this case is basically meaningless.

In this PR, I have set up the epsabs=1e-4 and epsrel=1e-3 so that the integral error is either smaller than before (epsabs reduction) or, at least, three significant digits smaller than the integral value.

I believe that this allows for a better balance between precision and steps taken than before while having an epsrel more meaningful, insofar as integrals that involve really large values do not require the tight precision dictated by absrel.

Nonetheless, I would like to hear your opinions on the matter. If this comment did not made the issue clear enough, please do not hesitate in telling me to reword it.

Copy link
Member

Choose a reason for hiding this comment

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

@phmbressan , I understood your situation and I think this is totally fine.

As a final user, I don't see any differences in the code if you chose one kind of error or another (abs/rel).

Copy link
Member

@MateusStano MateusStano left a comment

Choose a reason for hiding this comment

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

There are some very specific changes here, but they all seem to make sense, so good to go!

@Gui-FernandesBR
Copy link
Member

I have some comments. but didn't manage to find time yet. gonna comment here soon

Copy link
Member

Choose a reason for hiding this comment

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

I took some time to understand the changes, primarily due to my limited familiarity with certain geometric concepts. However, from what I gathered, it seems you've restructured the Jacobian implementation. While the style differs, I trust the outcome remains consistent with the previous approach. Great work.

Adding a few more docstrings would allow for a faster comprehension here.

Copy link
Member

@Gui-FernandesBR Gui-FernandesBR left a comment

Choose a reason for hiding this comment

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

Good work, just commented some observations.

@MateusStano MateusStano merged commit 82767ea into develop Oct 7, 2023
@Gui-FernandesBR Gui-FernandesBR deleted the enh/solid-ode-jacobian branch October 9, 2023 04:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Function Everything related to the Function class Motors Every propulsion related issue or PR Tests Regarding Tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants