BUG: Parachute Pressures not being Set before All Info.

[phmbressan]

Pull request type

• Code changes (bugfix, features)

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
• CHANGELOG.md has been updated (if relevant)

Current behavior

The method export_pressure incorrectly calls the lists: parachute.clean_pressure_signal and noisy_pressure_signal as if they were functions. Furthermore, this data is not set before the call of the all_info, which could lead to invalid exports if it was working.

The issue when trying to export the pressure was:

   3091 for parachute in self.rocket.parachutes:
   3092     for t in time_points:
-> 3093         p_cl = parachute.clean_pressure_signal(t)
   3094         p_ns = parachute.noisy_pressure_signal(t)
   3095         file.write(f"{t:f}, {p_cl:.5f}, {p_ns:.5f}\n")

TypeError: 'list' object is not callable

New behavior

The employed fix was to call the method that set these values not in the all_info, but after Flight computations. On top of that, the incorrect list calls were substituted by their _function counterparts.

Now the data is being exported correctly:

0.000000, 85639.55296, 85652.28657
1.000000, 85228.90121, 85223.88463
...

Breaking change

• No

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (223d598) 71.15% compared to head (a3e49a2) 71.23%.
Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #534      +/-   ##
==========================================
+ Coverage   71.15%   71.23%   +0.08%     
==========================================
  Files          55       55              
  Lines        9259     9259              
==========================================
+ Hits         6588     6596       +8     
+ Misses       2671     2663       -8     

Flag	Coverage Δ
unittests	71.23% <100.00%> (+0.08%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Gui-FernandesBR]

Well done!

I think we should adopt the Function.savetxt to do the same job once we finish its PR.

Nevertheless, for this hotfix I think we are fine with the solution, thanks.

[giovaniceotto]

Should we expect any significant increase in simulation execution speed since we are adding a new call inside Flight.__init__?

[phmbressan]

Should we expect any significant increase in simulation execution speed since we are adding a new call inside Flight.__init__?

That could be tested, of course, but I don't think so. The method basically makes two Function instances per parachute if I recall correctly.

Unless the simulation has unusualy too many points or a lot of parachutes, the time spend should be in the ms range.

Again, that could be easily tested later. This solution is not the best one, but I do think it involves the least amount of changes for a Hotfix.

I am open to other suggestions.

[Gui-FernandesBR]

Should we expect any significant increase in simulation execution speed since we are adding a new call inside Flight.__init__?

That could be tested, of course, but I don't think so. The method basically makes two Function instances per parachute if I recall correctly.

Unless the simulation has unusualy too many points or a lot of parachutes, the time spend should be in the ms range.

Again, that could be easily tested later. This solution is not the best one, but I do think it involves the least amount of changes for a Hotfix.

I am open to other suggestions.

I reviewed it again and I don't think it's a problem. The new behavior is not prejudicial in terms of code performance.

[Gui-FernandesBR]

In order to prevent future errors, we ideally should implement unit test for the methods we fix in hotfixes.
I did that in my last commit a3e49a2.

@phmbressan please review the commit and feel free to proceed and merge.

[phmbressan]

In order to prevent future errors, we ideally should implement unit test for the methods we fix in hotfixes.
I did that in my last commit a3e49a2.

@phmbressan please review the commit and feel free to proceed and merge.

Since I opened the PR GitHub does not allow me to submit an approval.

[MateusStano]

Good! The export pressure function has broken a million times before. Its good that we finally have a test for it!