Motor: update type hints and tests #253
Conversation
reilleya
left a comment
reilleya
left a comment
There was a problem hiding this comment.
A bunch of thoughts! Thanks for this!
motorlib/motor.py
Outdated
| using the pressure to determine how the motor will regress in the given timestep at the current pressure. | ||
| def runSimulation(self, callback=None) -> SimulationResult: | ||
| """ | ||
| Runs a simulation of the motor and returns a simRes instance with the results. |
There was a problem hiding this comment.
I guess breaking these up ("google style"?) could make them easier to read, but I don't think they were all that long to begin with. Maybe revert this for now?
motorlib/motor.py
Outdated
| """ | ||
| if not self.propellant: | ||
| raise ValueError( | ||
| "Ideal pressure cannot be calculated. Propelant is missing." |
There was a problem hiding this comment.
Nitpick: propelant -> propellant
| optionally be passed in to save time on motors where calculating surface area is expensive. | ||
| """ | ||
| if not self.propellant: | ||
| raise ValueError( |
There was a problem hiding this comment.
An exception is correct here rather than a simAlert. simAlerts are intended for cases where the user has done something that they need to be alerted about. If this function is ever called without a propellant set, it is likely a software bug that should bubble up as an exception.
motorlib/motor.py
Outdated
| """Calculates the mach number in the core of a grain for a given chamber pressure and mass flux.""" | ||
| if not self.propellant: | ||
| raise ValueError( | ||
| "Ideal pressure cannot be calculated. Propelant is missing." |
There was a problem hiding this comment.
Nitpick: Ideal pressure -> Mach number, Propelant -> Propellant
motorlib/motor.py
Outdated
| simRes.channels["volumeLoading"].addData( | ||
| 100 * (1 - (self.calcFreeVolume(perGrainReg) / motorVolume)) | ||
| ) | ||
| simRes.channels["massFlow"].addData([0] * len(self.grains)) |
There was a problem hiding this comment.
This feels like a really minor potential performance improvement given that this isn't in a loop or anything. Maybe not worth the diff?
motorlib/motor.py
Outdated
| for g, r in zip(self.grains, perGrainReg) | ||
| ] | ||
| ) | ||
| if callback( |
There was a problem hiding this comment.
I like a lot of what ruff has done, but this just reads worse to me. Absolutely no reason to split a line this short with one function call with one arg over so many lines... I'll look into if there's a way to configure ruff.
| maximumRefDiameter = 6.6 | ||
|
|
||
| # Max core Mach number subsonic limit | ||
| machSubsonicLimit: float = 1.0 |
There was a problem hiding this comment.
This feels a little bit silly because this is always going to be exactly 1.0. Maybe only worth doing if we think we are going to change the type someday? Certainly not going to have to update this value in multiple places. Up to you if you want to keep it or not.
There was a problem hiding this comment.
My main point here is to update raw float value into meaningful constant, which from my prospective makes code easier to read and understand, even if it looks silly :)
motorlib/utils.py
Outdated
| @@ -0,0 +1,52 @@ | |||
| """Module for utils functions to prevent reassign values from an outer scope.""" | |||
There was a problem hiding this comment.
Not sure I follow, can you give an example of what this is trying to avoid?
There was a problem hiding this comment.
I apologize for the lack of clarity. My intent with this module description is not to implement a language feature, but to enforce a code convention that avoids a specific type of subtle bug and confusion.
| ) -> float: | ||
| """Calculates the objective function for finding the Mach number. | ||
| This function represents a rearranged form of the isentropic mass flow equation |
akarev0
force-pushed
the
refactoring/motor-module-type-hints
branch
from
651bfe9 to
0a5c3bf
Compare
2 participants
Motor module updates.
self.propellantbecause it could be None and could cause errors (I have added ValueError but I might need to be converted into SimAllert, @reilleya wdyt?)