-
-
Notifications
You must be signed in to change notification settings - Fork 194
ENH: variable gravity #338
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really understand what is going on here. Does the gravity only vary if the user passes gravity = "variable"
? Shouldn't it be calculated just by getting the elevation? The gravity argument should be an optional argument that sets it to a constant value, just in case anyone wants to try to simulate another planet or something.
Am I getting something wrong here?
…ay instead of Function.
- Current fix works, but is not elegant. Perhaps pending Function Operations modifications.
I believe this PR may be reaching its final steps. Just a few things that I wanted to point out before finishing reviews and merging:
Gravity
Nota bene: the fin flutter tests still do not pass for me locally. |
Hey, seems that in this case the test actually did its job. The change in the gravity model actually changed the simulation by just enough to make the test break. It seems flutter analysis is very very sensitive I think I fixed it now, please just try to run it locally to make sure |
@MateusStano, I see you changed the values for the fin flutter test. Shouldn't we keep a test with the old values, but using constant gravity, just to be safe? To be clear, I mean we should keep both tests: the old one and the new one. |
rocketpy/Flight.py
Outdated
@@ -2121,8 +2121,7 @@ def potentialEnergy(self): | |||
# Redefine totalMass time grid to allow for efficient Function algebra | |||
totalMass = deepcopy(self.rocket.totalMass) | |||
totalMass.setDiscreteBasedOnModel(self.z) | |||
# TODO: change calculation method to account for variable gravity | |||
potentialEnergy = totalMass * self.env.gravity.compose(self.z) | |||
potentialEnergy = totalMass * self.env.gravity.compose(self.z) * self.z |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am afraid this formula,
Reference: https://en.wikipedia.org/wiki/Gravitational_energy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I implemented this in 90b2bdb with the following code:
potentialEnergy = (
GM
* totalMass
* (1 / (self.z + self.env.earthRadius) - 1 / self.env.earthRadius)
)
So what we are calculating here is the potential energy in relation to the sea level. All good now right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request type
Please check the type of change your PR introduces:
Pull request checklist
Please check if your PR fulfills the following requirements, depending on the type of PR:
ReadMe, Docs and GitHub maintenance:
Code base maintenance (refactoring, formatting, renaming):
black rocketpy
) has passed locally and any fixes were madepytest --runslow
) have passed locallyCode base additions (for bug fixes / features):
black rocketpy
) has passed locally and any fixes were madepytest --runslow
) have passed locallyWhat is the current behavior?
currently gravity is a constant equal to 9,80665 m/s²
What is the new behavior?
Now, env.g is a callable that returns the gravity given the height.
On the other hand, env.standard_gravity will store the constant value of 9,80665 m/s².
Does this introduce a breaking change?
Other information
Enter text here...