4
\$\begingroup\$

There is this code in which the calculation of the centrifugal nozzle of a liquid rocket engine is performed. All basic calculations take place in the Injector class. The AngularValues class stores the value of the right angle required for the code to work. The ConstructiveTypes class stores the type of nozzle being designed.

I'm thinking is it worth dividing the code into classes for calculation by type of construction, or is such code quite readable?

I also noticed that in cases when all functions are called sequentially, then many functions are executed more than once. Maybe it's worth caching using the functools library?

All formulas from this code, as well as possibly some additional information, are found in this book on pages 47 - 54 (book)

from enum import Enum
from math import pi, sqrt, log, atan, cos
from dataclasses import dataclass

class AngularValues(Enum):
    RIGHT_ANGLE = 90


class ConstructiveTypes(Enum):
    CENTRIFUGAL_INJECTOR = "CENTRIFUGAL"
    SCREW_INJECTOR = "SCREW"


@dataclass(frozen=True, slots=True)
class Injector:
    outer_diameter_injector: float
    side_wall_thickness_injector: float
    number_input_tangential_holes: float
    diameter_input_tangential_holes: float
    length_input_tangential_holes: float
    relative_length_twisting_chamber: float
    diameter_injector_nozzle: float
    relative_length_injector_nozzle: float
    angle_nozzle_axis: float
    mass_flow_rate: float
    viscosity: float
    cross_sectional_area_one_passage_channel: float
    density_fuel_component_front_injector: float
    density_combustion_products: float
    surface_tension_coefficient: float

    injector_type: str

    @property
    def diameter_twisting_chamber_injector(self) -> float:
        """Возвращает диаметр камеры закручивания центробежной форсунки"""
        return self.outer_diameter_injector - 2 * self.side_wall_thickness_injector

    @property
    def relative_length_tangential_hole(self) -> float:
        """Возвращает отношение длины входного тангенциального к его диаметру"""
        return self.length_input_tangential_holes / self.diameter_input_tangential_holes

    @property
    def length_twisting_chamber(self) -> float:
        """Возвращает длину камеры закручивания центробежной форсунки"""
        return self.relative_length_twisting_chamber * self.diameter_twisting_chamber_injector

    @property
    def radius_twisting_chamber_injector(self) -> float:
        """Возвращает радиус камеры закручивания центробежной форсунки"""
        return self.diameter_twisting_chamber_injector / 2

    @property
    def radius_input_tangential_holes(self) -> float:
        """Возвращает радиус входных тангенциальных отверстий"""
        return self.diameter_input_tangential_holes / 2

    @property
    def radius_tangential_inlet(self) -> float:
        """Возвращает величину радиуса, на котором расположена ось входного тангенциального отверстия от оси форсунки"""
        return self.radius_twisting_chamber_injector - self.radius_input_tangential_holes

    @property
    def length_injector_nozzle(self) -> float:
        """Возвращает длину сопла форсунки"""
        return self.relative_length_injector_nozzle * self.diameter_injector_nozzle

    @property
    def radius_injector_nozzle(self) -> float:
        """Возвращает радиус сопла форсунки"""
        return self.diameter_injector_nozzle / 2

    @property
    def reynolds_number(self) -> float:
        """Возвращает число Рейнольдса"""
        return (4 * self.mass_flow_rate) / (pi * self.viscosity * self.diameter_input_tangential_holes
                                            * sqrt(self.number_input_tangential_holes))

    @property
    def coefficient_friction(self) -> float:
        """Возвращает коэффициент трения"""
        return 10 ** ((25.8 / (log(self.reynolds_number, 10)) ** 2.58) - 2)

    @property
    def geometric_characteristics_screw_injector(self) -> float:
        """Возвращает геометрическую характеристику шнековой форсунки"""
        return (pi * self.radius_tangential_inlet * self.radius_injector_nozzle) / \
               (self.number_input_tangential_holes * self.cross_sectional_area_one_passage_channel)

    @property
    def geometric_characteristics_centrifugal_injector(self) -> float:
        """Возвращает геометрическую характеристику центробежной форсунки"""
        if self.angle_nozzle_axis == AngularValues.RIGHT_ANGLE.value:
            return (self.radius_tangential_inlet * self.radius_injector_nozzle) / \
                   (self.number_input_tangential_holes * self.radius_input_tangential_holes ** 2)
        else:
            return (self.radius_tangential_inlet * self.radius_injector_nozzle) / \
                   (
                           self.number_input_tangential_holes * self.radius_input_tangential_holes ** 2) * \
                   self.angle_nozzle_axis

    @property
    def equivalent_geometric_characteristic_injector(self) -> float:
        """Возвращает эквивалентную геометрическую характеристику"""
        if self.injector_type == ConstructiveTypes.SCREW_INJECTOR.value:
            geometric_characteristics = self.geometric_characteristics_screw_injector
        else:
            geometric_characteristics = self.geometric_characteristics_centrifugal_injector

        return geometric_characteristics / (1 + self.coefficient_friction / 2 * self.radius_tangential_inlet *
                                            (self.radius_tangential_inlet + self.diameter_input_tangential_holes -
                                             self.radius_injector_nozzle))

    @property
    def ratio_live_section_injector_nozzle(self) -> float:
        """Возвращает коэффициент живого сечения сопла форсунки"""
        return 1 / ((self.equivalent_geometric_characteristic_injector / (2 * sqrt(2)) +
                    sqrt(self.equivalent_geometric_characteristic_injector**2 / 8 - 1 / 27))**(1/3) +
                    (self.equivalent_geometric_characteristic_injector / (2 * sqrt(2)) -
                    sqrt(self.equivalent_geometric_characteristic_injector**2 / 8 - 1 / 27))**(1/3))**2

    @property
    def flow_rate_centrifugal_injector(self) -> float:
        """Возвращает коэффициент расхода центробежной форсунки"""
        return self.ratio_live_section_injector_nozzle * sqrt(self.ratio_live_section_injector_nozzle /
                                                              (2 - self.ratio_live_section_injector_nozzle))

    @property
    def average_angle_spray_torch(self) -> float:
        """Возвращает средний угол факела распыла"""
        return atan(2 * self.flow_rate_centrifugal_injector * self.equivalent_geometric_characteristic_injector /
                    sqrt((1 + sqrt(1 - self.ratio_live_section_injector_nozzle))**2 - 4 *
                         self.flow_rate_centrifugal_injector**2 *
                         self.equivalent_geometric_characteristic_injector**2))

    @property
    def injector_nozzle_area(self) -> float:
        """Возвращает площадь сопла форсунки"""
        return pi * self.diameter_injector_nozzle ** 2 / 4

    @property
    def pressure_drop_front_injector(self) -> float:
        """Возврашает перепад давления на форсунке, для обеспечения необходимого расхода компонента через форсунку"""
        return self.mass_flow_rate**2 / (2 * self.density_fuel_component_front_injector *
                                         self.flow_rate_centrifugal_injector**2 * self.injector_nozzle_area**2)

    @property
    def radius_vortex_outlet_section_injector(self) -> float:
        """Возвращает радиус вихря жидкости или воздушного вихря в выходном сечении форсунки"""
        return self.radius_injector_nozzle * sqrt(1 - self.ratio_live_section_injector_nozzle)

    @property
    def area_live_section_injector_nozzle(self) -> float:
        """Возвращает площадь живого сечения сопла форсунки"""
        return self.ratio_live_section_injector_nozzle * self.injector_nozzle_area

    @property
    def average_value_axial_velocity_outlet_injector(self) -> float:
        """Возвращает среднее значение осевой скорости на выходе из форсунки"""
        return self.mass_flow_rate / (self.density_fuel_component_front_injector *
                                      self.area_live_section_injector_nozzle)

    @property
    def average_value_absolute_velocity_outlet_injector(self) -> float:
        """Возвращает среднее значение абсолютной скорости на выходе из форсунки"""
        return self.average_value_axial_velocity_outlet_injector / cos(self.average_angle_spray_torch)

    @property
    def thickness_veil_outlet_injector(self) -> float:
        """Возвращает толщину пелены на выходе из форсунки"""
        return self.radius_injector_nozzle - self.radius_vortex_outlet_section_injector

    @property
    def weber_criterion(self) -> float:
        """Возвращает критерий Вебера"""
        return self.density_combustion_products * self.average_value_absolute_velocity_outlet_injector**2 * \
            self.diameter_injector_nozzle / self.surface_tension_coefficient

    @property
    def laplace_criterion(self) -> float:
        """Возвращает критерий Лапласа"""
        return self.density_fuel_component_front_injector * self.thickness_veil_outlet_injector * \
            self.surface_tension_coefficient / self.viscosity

    @property
    def media_diameter_spray_torch_droplets(self) -> float:
        """Возвращает медианный диаметр образовавшихся капель в факеле распыла форсунки"""
        return 269 * self.laplace_criterion**-0.35 * ((self.weber_criterion * self.density_combustion_products) /
                                                      self.density_fuel_component_front_injector)**0.483
\$\endgroup\$
0

3 Answers 3

7
\$\begingroup\$

Thank you for citing a reference.

worth dividing ... by type of construction

The code is quite readable as it is. But we have 26 derived properties, so yes, if you can break out a couple of categories that would be helpful. For example, the {centrifugal, screw} distinction would likely map nicely to a pair of classes.

number of parameters

I see 16 arguments to the __init__ constructor. None of them are defaulted. Presumably it's necessary to supply all of them to the ctor, given that they're immutable, not None, and can't be changed later.

That seems like a lot.

I think I see about three different parameter groups. The big distinction is "what do I have?" versus "how am I operating it?", so things like viscosity and flow rate might sensibly go into a companion dataclass.

And then looking at the physical construction, we might choose to distinguish between nozzle and chamber. Or we might think of "original equipment" and then "equipment with holes drilled", given that there is some flexibility to arrangement of holes. I'm just looking for simpler subassemblies, is all. Ideally there are some parameters which remain fixed across a bunch of instances, and we can put those in a base class which the others build upon.

docstrings

Uniform use of @property works great for this codebase. On the one hand the identifiers are a bit on the verbose side, but OTOH they are laudably clear. Keep them.

Often I will lament that def foo(...) does complex stuff and needs a docstring to explain it. But here the various properties are extremely clear, and their accompanying docstrings tend to just repeat what the identifier already told us. It's redundant, and not pulling its own weight. Consider discarding many of those docstrings.

Consider doing the occasional rename to clarify properties. For example, when I read relative_length_tangential_hole I think it's going to be a dimensionless quantity, and that's the question I have in mind when I read the docstring to verify. Maybe rename to tangential_hole_length_ratio so the question does not arise? Also, I'm not familiar with the terms of art in this field, but maybe "swirl chamber" or "mixing chamber" is more often used than "twisting chamber"?

simple relationship

The relationship between radius and diameter is extremely simple and well known. The formulae tend to be phrased more often in terms of radius. Maybe don't keep track of diameter at all?

I found it slightly odd to phrase injector_nozzle_area in terms of diameter rather than more naturally using radius.

BTW, style nit: Avoid using \ backwhack when feasible. In many places you have a return ( ... ) expression, more than a line long, which has "extra" ( ) parens, very nice. There's no need for backwhack at end of line there, since the parser will keep looking for that closing ) parenthesis. Black can help with such minutiae.

cache

I'm a big fan of @lru_cache, you could certainly use that. Given the simple access pattern for a frozen instance, the simpler @cache would be even better.

But for all those immutable properties, @cached_property would make the most sense.

Total speedup won't be much, in any case, since the computations are so simple and are free of loops. Couldn't hurt, though.

slots

We request slots=True, yet make no references to __slots__. Consider simplifying the class by just using the default slots setting.


Overall this looks good. You could certainly ship it as-is.

\$\endgroup\$
5
  • \$\begingroup\$ Can @cached_property be used when slots are also being used? When I try it I get, for example, TypeError: No '__dict__' attribute on 'Foo' instance to cache 'getx' property. \$\endgroup\$
    – Booboo
    Commented Jun 8 at 16:27
  • \$\begingroup\$ @Booboo They're not compatible. I recommended that OP use the default slots setting, as there's nothing in that code to motivate messing with slots. \$\endgroup\$
    – J_H
    Commented Jun 8 at 16:31
  • \$\begingroup\$ Sorry about that. I had stopped reading and commented as soon as I saw you recommend @cached_property. Perhaps you should say, "But for all those immutable properties, @cached_property would make the most sense provided you use the default slots setting (see next section)." No big deal and thanks. \$\endgroup\$
    – Booboo
    Commented Jun 8 at 16:48
  • 1
    \$\begingroup\$ Re docstrings: the language is more problematic than the manner of their use IMO. Yep, many are redundant and just repeat identifier names, but such use may be justified: e.g. they are sphinx-friendly, allowing to build beautiful docs for this package in 10 minutes. Docs with undocumented properties always look raw and incomplete, despite clear identifiers used. On the other hand, writing everything in Russian prevents anyone from other culture from using it, similar to some libraries with Chinese-only documentation. For me this is really a red herring, usually making me discard the library. \$\endgroup\$
    – STerliakov
    Commented Jun 8 at 20:33
  • 1
    \$\begingroup\$ yet make no references to __slots__ - I probably agree given the context (this class likely isn't used in performance-critical code or instantiated many times), but in general slots=True is a sensible default: you get faster attribute access and lower memory footprint for free. Using cached_property can be a good reason to not add slots, but the original OP code is applying slots correctly IMO. \$\endgroup\$
    – STerliakov
    Commented Jun 8 at 20:40
5
\$\begingroup\$

First of all: thank you! This code is in good shape overall, I wish everything I work with could look like this.

Now, a few more comments to complement @J_H answer:

Enum? String?

You define a beautiful enum ConstructiveTypes, but then downcast its usage to str everywhere for some reason. It'd be much cleaner if you annotated injector_type: ConstructiveTypes and then compare by self.injector_type is ConstructiveTypes.SCREW_INJECTOR: such usage conveys semantics. Looking at your dataclass alone, I don't see whether Injector(..., injector_type='welcome') is acceptable.

Line length and breaks

I understand that many lines are long just because the identifiers are long. Unfortunately, this fact doesn't make them easier to read. My editor is sometimes in 2-column mode, sometimes even in 3-column, and adhering to 80 or 88 chars per line really helps fitting them on screen. However, your limit is apparently 120 chars per line (at least you adhere to it consistently), so it's fine as long as it works for you.

Also, \-continuation is usually discouraged: it's easy to mess with and sometimes hard to grasp.

Consider using ruff or black to format your code automatically and never think again about this. Ideally, set some formatter and linter as a pre-commit hook to run whenever you commit your code.

Docstrings

PEP-257 recommends that

The docstring is a phrase ending in a period. It prescribes the function or method’s effect as a command (“Do this”, “Return that”), not as a description; e.g. don’t write “Returns the pathname …”.

So the use of "Возвращает" is generally discouraged. Please also consider writing documentation in English: the future maintainer will thank you. People from other cultures also want to be able to read your documentation. Also it's just easier to edit, no language switch is necessary and no context switch thus happens.

Identifier naming

Most of your attributes are clearly named, the names are understandable without having to consult the docs. However, their style reminds me of Hungarian notation - you seem to use words in some unnatural order. E.g. outer_diameter_injector is not something you can say in English: it'd be injector_outer_diameter or outer_diameter_of_injector (with the former preferred, I guess, just because it's shorter).

Pay attention to singular/plural usage: when an identifier is named in plural, it is expected to be some container/iterable/sequence, so geometric_characteristics_centrifugal_injector(self) -> float is a bit confusing.

Dataclass usage

This class is a perfect candidate for a dataclass, thanks! Consider making it kw_only=True (python 3.10+ only), because passing 16 parameters must never happen positionally.

Stdlib usage

You have

math.log(self.reynolds_number, 10)

which is fine, but could be just

math.log10(self.reynolds_number)

Imports

You have many math imports. It is OK to go this way, but in case of "multitools" in standard library "namespaced" import style is usually preferred: import math and then math.log(...). Why? First, now you have a global constant pi that violates the naming convention (should be PI). I don't know why Python maintainers decided like this. Second, if you were to replace log with log10 (see above), you'll have to change one more place than necessary.

Testing

Consider writing a simple test case that freezes output for both injector types with some fixed parameters. This way you'll be able to refactor easily without worrying whether you can break something. Ideally, such tests should be based on some external source - if there is some example of calculation in source, write a test to confirm that you get the same result. Beware of floating-point precision!

Correctness

Here's a property that looks suspicious to me:

    @property
    def geometric_characteristics_centrifugal_injector(self) -> float:
        """Возвращает геометрическую характеристику центробежной форсунки"""
        if self.angle_nozzle_axis == AngularValues.RIGHT_ANGLE.value:
            return (self.radius_tangential_inlet * self.radius_injector_nozzle) / \
                   (self.number_input_tangential_holes * self.radius_input_tangential_holes ** 2)
        else:
            return (self.radius_tangential_inlet * self.radius_injector_nozzle) / \
                   (self.number_input_tangential_holes * self.radius_input_tangential_holes ** 2) * \
                   self.angle_nozzle_axis

I aligned the lines to demonstrate the issue. Two formulae in branches differ only in the last multiplier. That multiplier is an angle in degrees, judging by enum value. So do these values really differ by two orders of magnitude with angles of 90 and 89.9 degrees?

Float math

You have the following check (angle_nozzle_axis is defined as float):

if self.angle_nozzle_axis == AngularValues.RIGHT_ANGLE.value:
    ...

Now, have fun in REPL:

>>> 90.0 == 90
True
>>> (0.3+0.6)*100==90
False

So if this angle was coming from some calculation, you may choose the wrong branch.

Code specialization

You have the following properties:


    @property
    def geometric_characteristics_screw_injector(self) -> float:
        """Возвращает геометрическую характеристику шнековой форсунки"""
        return (pi * self.radius_tangential_inlet * self.radius_injector_nozzle) / \
               (self.number_input_tangential_holes * self.cross_sectional_area_one_passage_channel)

    @property
    def geometric_characteristics_centrifugal_injector(self) -> float:
        """Возвращает геометрическую характеристику центробежной форсунки"""
        if self.angle_nozzle_axis == AngularValues.RIGHT_ANGLE.value:
            return (self.radius_tangential_inlet * self.radius_injector_nozzle) / \
                   (self.number_input_tangential_holes * self.radius_input_tangential_holes ** 2)
        else:
            return (self.radius_tangential_inlet * self.radius_injector_nozzle) / \
                   (
                           self.number_input_tangential_holes * self.radius_input_tangential_holes ** 2) * \
                   self.angle_nozzle_axis

If I understand the intent correctly, these properties are only applicable to one of types. You do not want Injector(..., injector_type=ConstructiveTypes.SCREW_INJECTOR.value).geometric_characteristics_centrifugal_injector to succeed. This can be expressed as


    @property
    def geometric_characteristics_screw_injector(self) -> float:
        """Возвращает геометрическую характеристику шнековой форсунки"""
        if self.injector_type != ConstructiveTypes.SCREW_INJECTOR.value:
            raise AttributeError("Only applicable to screw injectors.")
        return (pi * self.radius_tangential_inlet * self.radius_injector_nozzle) / \
               (self.number_input_tangential_holes * self.cross_sectional_area_one_passage_channel)

But since you have other properties with similar approach (flow_rate_centrifugal_injector and everything that depends on it?), this is no longer feasible: a better design would be to split Injector into BaseInjector with common properties and two subclasses (CentrifugalInjector and ScrewInjector), as you have suggested in the question. This will ensure that no unrelated properties can be accidentally accessed. This is even more important if there some attributes specific to one of these types - then asking user to provide an unnecessary parameter is always a bad idea.

\$\endgroup\$
4
\$\begingroup\$

Even without understanding the matter at hand, or the algos involved, if we look at it purely from a programming view point I believe some properties could be consolidated.

A function can return more than just one value, so you could return a tuple of values or a namedtuple. See the doc for examples, that is quite easy to grasp. You already use Enum in your code so you already know how it works.

I guess the technique can be used for example to consolidate the multiple geometric_characteristics* properties defined in your class. This would also allow you to use shorter function/property names.

Likewise, you can use namedtuple (or any type actually) as function argument when that makes sense, rather than throw a bunch of variables that could be grouped in some way.

As said already, consider using black, isort etc to format the code automatically. I use pre-commit (git hooks), which is quite convenient and improves code quality, enforces PEP8 style etc. You can add more tools like bandit etc to check for possible weaknesses in your code and more.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.