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

Unit test motor class #80

Merged
merged 14 commits into from Sep 12, 2021
Merged

Unit test motor class #80

merged 14 commits into from Sep 12, 2021

Conversation

PatrickSampaioUSP
Copy link
Contributor

@PatrickSampaioUSP PatrickSampaioUSP commented Aug 8, 2021

Pull request checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)
  • Build (python setup.py install) was run locally and any changes were pushed
  • Lint (black rocketpy) has passed locally and any fixes were made for failures
  • All tests (pytest --runslow) have passed locally

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

Does this introduce a breaking change?

  • Yes
  • No

Other information

Added 95% coverage unit test for SolidMotor class. In general lines the tests are validating the values calculated by the SolidMotor classes, but not verifying every single value, but doing a more analytical assert to check if the results make sense.

@PatrickSampaioUSP PatrickSampaioUSP added the Enhancement New feature or request, including adjustments in current codes label Aug 8, 2021
@@ -689,7 +711,7 @@ def exportEng(self, fileName, motorName):
)

# Write thrust curve data points
for item in self.thrust.source[:-1, :]:
for item in self.thrust.source[:-1, :][1:]:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@giovaniceotto Could you validate this? The previous code was adding two lines with 0.0 0.0 in the .eng file

Copy link
Member

Choose a reason for hiding this comment

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

Just checked here and I couln't reproduce it. Trying the following block of code with your implementation, I get a file missing the initial 0.0, 0.0 point.

Code

from rocketpy import SolidMotor

Pro75M1670 = SolidMotor(
    thrustSource="data/motors/Cesaroni_M1670.eng",
    burnOut=3.9,
    grainNumber=5,
    grainSeparation=5/1000,
    grainDensity=1815,
    grainOuterRadius=33/1000,
    grainInitialInnerRadius=15/1000,
    grainInitialHeight=120/1000,
    nozzleRadius=33/1000,
    throatRadius=11/1000,
    interpolationMethod='linear'
)

Pro75M1670.exportEng('test.csv', 'Pro75M1670')

Result file test.csv

Pro75M1670 66.0 625.0 0 2.96 2.96 PJ 
0.0550 100.000
0.0920 1500.000
0.1000 2000.000
0.1500 2200.000
0.2000 1800.000
0.5000 1950.000
1.0000 2034.000
1.5000 2000.000
2.0000 1900.000
2.5000 1760.000
2.9000 1700.000
3.0000 1650.000
3.3000 530.000
3.4000 350.000
3.9000 0.000

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are correct.

What motivated me to make this change is that the importEng method already initialize the dataPoints variable with a [[0, 0]]. Which results in our exported file to be read with two rows with zeros, what I thinks should not be ideal. What do you think it is the best solutions for this issue?

I see that we either remove the initial [0, 0] from our exported file or we do not assume the [0, 0] in the dataPoints variable. A third option would be to check if the input file already has a [0, 0] row.

@PatrickSampaioUSP
Copy link
Contributor Author

@giovaniceotto We are going to save the .eng file without the initial 0,0 and the importEng will add the initial 0,0 in the datapoints, right? The code is already producing this behaviour.

Copy link
Member

@giovaniceotto giovaniceotto left a comment

Choose a reason for hiding this comment

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

Seems like everything is working as expected!

@giovaniceotto giovaniceotto merged commit 0aa4d54 into master Sep 12, 2021
@giovaniceotto giovaniceotto deleted the unit_test_motor_class branch September 12, 2021 16:48
@Gui-FernandesBR Gui-FernandesBR added Tests Regarding Tests C.I. Continuous Integration (Workflows and actions) labels Jun 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C.I. Continuous Integration (Workflows and actions) Enhancement New feature or request, including adjustments in current codes Tests Regarding Tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants