Injector Model and Basic Structure for General Model

[shillcoat]

Unit tests created and passing for all fully implemented segments. I've attached partial documentation (only covers the injector model right now) for reference. I'm going to be redoing that on Overleaf though which is why it's not properly in the repository.
OpenThrustPartialDocumentation.pdf

---

This change is 

[shillcoat]

This PR should only have the actual relevant files now, sorry about that.

[chrissank]

Reviewed 17 of 100 files at r1, 1 of 83 files at r2.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @jacobdeery, @shillcoat, and @wendi-yu)

---

configReader.py, line 8 at r2 (raw file):

Quoted 5 lines of code…

    newModel.V = (float(config["Rocket Dimensions"]["oxidizer tank volume (l)"])) / 1000.0  # L to m^3
    newModel.nozThroatArea = (float(config["Rocket Dimensions"]["nozzle throat area (cm^2)"])) * 0.0001  # cm^2 to m^2
    newModel.nozExitArea = (float(config["Rocket Dimensions"]["nozzle exit area (cm^2)"])) * 0.0001  # cm^2 to m^2
    newModel.Ac = (float(config["Rocket Dimensions"]["injector area (cm^2)"])) * 0.0001  # cm^2 to m^2

If we have constants.py and have other measurements such as PSI to PASCAL in there, maybe these conversion factors could be there too even though they're pretty simple

---

constants.py, line 8 at r2 (raw file):

PSItoPASCAL = 6894.76  # Multiplying factor to convert from psi to Pascals
ATMOSPHERE_PSI = 14.7
CtoKELVIN = 273.15

PSI_TO_PASCAL, CELCIUS_TO_KELVIN? Just for conventions sake

---

getRPA.py, line 61 at r2 (raw file):

# TODO: Read from settings file to create RPA config file (store in rpaWrapper folder)

Is this a TODO destined the wrapper folder? In this case, do we want to remove it from the .gitignore so that this change gets tracked eventually? I think the idea was to .gitignore it for now, then once it's been added to the master branch, remove it then pull from master. I think this has been done now.

---

Models/injectorModel.py, line 35 at r2 (raw file):

Quoted 10 lines of code…

        # Dyer et. al factor. Always 1 for self-pressurizing fluid
        k = ((self.parent.P1 - self.parent.P2) / (self.parent.Pv1 - self.parent.P2)) ** 0.5
        W = (1 / (k + 1))
        self.mdot_SPI = self.Cd * self.Ac * (
                (2 * self.parent.rho1 * ((self.parent.P1 - self.parent.P2) * PSItoPASCAL)) ** 0.5)
        self.mdot_HEM = self.Cd * self.parent.rho2 * self.Ac * (
                (2 * (self.parent.h1 - self.parent.h2)) ** 0.5)
        if isinstance(self.mdot_HEM, complex):
            self.mdot_HEM = 0  # Case where h1<h2
        return (1 - W) * self.mdot_SPI + W * self.mdot_HEM

Might be good to comment more as this reading this is kind of difficult. If it comes from research, maybe a link to it or specifying the equations could be good.

[shillcoat]

Reviewable status: 14 of 18 files reviewed, 4 unresolved discussions (waiting on @chrissank, @jacobdeery, and @wendi-yu)

---

configReader.py, line 8 at r2 (raw file):

Previously, chrissank (Chris Sankey) wrote…

    newModel.V = (float(config["Rocket Dimensions"]["oxidizer tank volume (l)"])) / 1000.0  # L to m^3
    newModel.nozThroatArea = (float(config["Rocket Dimensions"]["nozzle throat area (cm^2)"])) * 0.0001  # cm^2 to m^2
    newModel.nozExitArea = (float(config["Rocket Dimensions"]["nozzle exit area (cm^2)"])) * 0.0001  # cm^2 to m^2
    newModel.Ac = (float(config["Rocket Dimensions"]["injector area (cm^2)"])) * 0.0001  # cm^2 to m^2

If we have constants.py and have other measurements such as PSI to PASCAL in there, maybe these conversion factors could be there too even though they're pretty simple

Done.

---

constants.py, line 8 at r2 (raw file):

Previously, chrissank (Chris Sankey) wrote…

PSItoPASCAL = 6894.76  # Multiplying factor to convert from psi to Pascals
ATMOSPHERE_PSI = 14.7
CtoKELVIN = 273.15

PSI_TO_PASCAL, CELCIUS_TO_KELVIN? Just for conventions sake

Done.

---

getRPA.py, line 61 at r2 (raw file):

Previously, chrissank (Chris Sankey) wrote…

# TODO: Read from settings file to create RPA config file (store in rpaWrapper folder)

Is this a TODO destined the wrapper folder? In this case, do we want to remove it from the .gitignore so that this change gets tracked eventually? I think the idea was to .gitignore it for now, then once it's been added to the master branch, remove it then pull from master. I think this has been done now.

Yes so we're keeping the RPA wrapper folder in the end, just added it in in a separate PR. This TODO is talking about a config file that I may have to create to work with the RPA wrapper, in which case OpenThrust will generate the file directly in the rpaWrapper folder. Only some subfolders are in the .gitignore, not the entire folder.

---

Models/injectorModel.py, line 35 at r2 (raw file):

Previously, chrissank (Chris Sankey) wrote…

        # Dyer et. al factor. Always 1 for self-pressurizing fluid
        k = ((self.parent.P1 - self.parent.P2) / (self.parent.Pv1 - self.parent.P2)) ** 0.5
        W = (1 / (k + 1))
        self.mdot_SPI = self.Cd * self.Ac * (
                (2 * self.parent.rho1 * ((self.parent.P1 - self.parent.P2) * PSItoPASCAL)) ** 0.5)
        self.mdot_HEM = self.Cd * self.parent.rho2 * self.Ac * (
                (2 * (self.parent.h1 - self.parent.h2)) ** 0.5)
        if isinstance(self.mdot_HEM, complex):
            self.mdot_HEM = 0  # Case where h1<h2
        return (1 - W) * self.mdot_SPI + W * self.mdot_HEM

Might be good to comment more as this reading this is kind of difficult. If it comes from research, maybe a link to it or specifying the equations could be good.

This kind of thing doesn't lend itself to explanation in code comments, but I did write up a fair bit going through it in a partially completed documentation file that I plan to move into a LaTeX editor at some point soon, which is why it itself isn't in the repository. I attached this in the initial comment/description when I submitted the PR though if you're interested.

[chrissank]

Reviewed 4 of 4 files at r3.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @jacobdeery, @shillcoat, and @wendi-yu)

a discussion (no related file):
Lets get another set of eyes on it incase I missed something in my initial review (just since it's so large), but other than that everything looks good to me!

---

---

Models/injectorModel.py, line 35 at r2 (raw file):

Previously, shillcoat (Sophie) wrote…

This kind of thing doesn't lend itself to explanation in code comments, but I did write up a fair bit going through it in a partially completed documentation file that I plan to move into a LaTeX editor at some point soon, which is why it itself isn't in the repository. I attached this in the initial comment/description when I submitted the PR though if you're interested.

I agree. I just saw the document you linked, my only suggestion is that maybe a comment pointing to said explanation would be good, but as long as it's got an explanation somewhere, it's cool with me.

[jacobdeery]

Reviewable status: all files reviewed, 24 unresolved discussions (waiting on @chrissank, @shillcoat, and @wendi-yu)

a discussion (no related file):

Previously, chrissank (Chris Sankey) wrote…

Lets get another set of eyes on it incase I missed something in my initial review (just since it's so large), but other than that everything looks good to me!

Apologies for the delay in reviewing!

---

---

configReader.py, line 2 at r3 (raw file):

import configparser
from constants import CM2_TO_M2, L_TO_M3

Nit: By PEP 8, there should be a new line in between these imports (standard library vs local import)

---

configReader.py, line 5 at r3 (raw file):

ReadSettings

this is PascalCase, but other functions are in camelCase (readSettings); please standardize on one or the other.

---

configReader.py, line 10 at r3 (raw file):

(

I don't think the outer layer of parens is needed for any of these calcs

---

constants.py, line 6 at r3 (raw file):

# Multiplying factor to convert from psi to Pascals

Comment isn't necessary, variable name is self-explanatory

---

databaseClass.py, line 3 at r3 (raw file):

from scipy.interpolate import interp1d
import csv
from constants import CENTIGRADE_TO_KELVIN

As with previous comment, should be:

import csv

from scipy.interpolate import interp1d

from constants import CENTIGRADE_TO_KELVIN

---

databaseClass.py, line 21 at r3 (raw file):

object

No need to explicitly inherit from object; this can be just class DataBase:

---

databaseClass.py, line 38 at r3 (raw file):

        self.s_V_NIST = []

    def buildNistSplines(self):

Is there any reason to not immediately call this function when the database is initialized?

---

databaseClass.py, line 42 at r3 (raw file):

        for row in csv.reader(open(self.path, 'r'), delimiter='\t'):
            if rowCount > -1:

Right now you open the file but never close it. This probably won't cause any real problems in this case, but it's best to avoid that. If you use a context manager (with open(self.path, 'r') as csvfile), the file will automatically close once it leaves scope.

---

databaseClass.py, line 78 at r3 (raw file):

if (T == 0 or T) and T <= 0:

prefer if (T is not None) and (T <=0); a bit more clear what the test is

---

databaseClass.py, line 82 at r3 (raw file):

        if rho <= 0:
            raise ValueError("Density is at or below 0")
        if (P == 0 or P) and P <= 0:

same as above

---

databaseClass.py, line 91 at r3 (raw file):

            props["T"] = T
            props["P"] = float(self.nistSplines_T["P"](T))
            propSet = lambda prop: float(self.nistSplines_T[prop](T))

This is cute

---

databaseClass.py, line 108 at r3 (raw file):

        props["h"] = props["h_V_NIST"]*props["X"] + props["h_L_NIST"]*(1-props["X"])
        props["s"] = props["s_V_NIST"]*props["X"] + props["s_L_NIST"]*(1-props["X"])
        props["state"] = 1

What does state represent?

---

getRPA.py, line 69 at r3 (raw file):

"rpaWrapper\\" + name

os.path.join("rpaWrapper", name) works on all platforms and lets you avoid the manual escaping

---

getRPA.py, line 97 at r3 (raw file):

object

Not necessary

---

getRPA.py, line 106 at r3 (raw file):

    # constructor from existing config file (add RPA section if required)
    def __init__(self, confPath):
        # self.conf = rpa.configFileLoad(encodeString(confPath))

Remove commented line? or is this necessary

---

main.py, line 4 at r3 (raw file):

# To get console output from RPA, change the 0 to a 1
getRPA.rpa.initializeWithPath("rpaWrapper\\" + getRPA.libraryVer + "\\resources", None, 0)

This line errors for me because the first argument isn't a c_char_p

---

requirements.txt, line 1 at r3 (raw file):

dataclasses

dataclasses is part of the standard library, so no need to add it here (but you should add scipy)

---

Vidar3RPA.cfg, line 1 at r3 (raw file):

version = 1.2;

Is this file actually for Vidar 3?

---

Models/injectorModel.py, line 21 at r3 (raw file):

        # Maybe do more research on handling this but for the moment not handled by model.
        if min(self.parent.P1, self.parent.Pv1) < self.parent.P2 or self.parent.P2 == self.parent.Pv1:

This condition might be a bit easier to read as: if self.parent.P1 < self.parent.P2 or self.parent.Pv1 <= self.parent.P2

---

UnitTests/test_configReader.py, line 3 at r3 (raw file):

import pytest
import configReader
from dataclasses import dataclass

from dataclasses import dataclass

import pytest

import configReader

---

UnitTests/test_configReader.py, line 24 at r3 (raw file):

pytest.approx

this is very cool, I was not aware it existed

---

UnitTests/test_databaseClass.py, line 3 at r3 (raw file):

import pytest
import databaseClass as db
from constants import NIST_PATH

same as previous comments

---

UnitTests/test_databaseClass.py, line 5 at r3 (raw file):

from constants import NIST_PATH

There's an extra newline here

---

UnitTests/test_databaseClass.py, line 13 at r3 (raw file):

class TestDB:

Nicely done splitting the tests into separate classes. This is something I should probably do for topside.

---

UnitTests/test_injectorModel.py, line 3 at r3 (raw file):

from Models.injectorModel import *
from dataclasses import dataclass
import pytest

same

[wendi-yu]

Reviewable status: all files reviewed, 30 unresolved discussions (waiting on @chrissank and @shillcoat)

a discussion (no related file):

Previously, jacobdeery (Jacob Deery) wrote…

Apologies for the delay in reviewing!

Yes sorry I was even later lol, looks really good overall though :0

---

---

configReader.py, line 11 at r3 (raw file):

    # Set rocket dimension settings
    newModel.V = (float(config["Rocket Dimensions"]["oxidizer tank volume (l)"])) * L_TO_M3
    newModel.nozThroatArea = (float(config["Rocket Dimensions"]["nozzle throat area (cm^2)"])) * CM2_TO_M2

Having the setting names hardcoded into here seems pretty easy to break accidentally, but I'm not sure if there's a better way to do it.

---

databaseClass.py, line 12 at r3 (raw file):

def calcQuality(rho, rho_V, rho_L):
    """
    Returns the factor for determining properties of the liquid/vapour mixture

Nit but I think docstrings are generally supposed in imperative, so "Return the factor..." instead of "Returns"

---

getRPA.py, line 66 at r3 (raw file):

    #  customizing an empty conf object
    name = fileName
    if len(name) == 0:

if name == ""? Unless I'm missing something lol

---

getRPA.py, line 78 at r3 (raw file):

    # Get from model the settings that were taken from the configuration file and use to create
    confFile.write('version = 1.2;\nname = "' + name +

Format strings and triple quotes might make this easier to format, something like

confFile.write(f"""\
version = 1.2;
name = {name};
info = This file was generated automatically by OpenThrust;
""")

---

Models/hybridModels.py, line 9 at r3 (raw file):

class GeneralModel(object):
    """
        General model implementation

I think single line docstrings can just all be on a single line with the triple quotes?

---

Models/hybridModels.py, line 50 at r3 (raw file):

        try:
            self.rho1 = self.m / self.V

Is the expected failure mode for this just that configReader.ReadSettings() didn't successfully read in a value for self.V, or is there something else? Maybe worth mentioning in a comment

---

Models/injectorModel.py, line 35 at r2 (raw file):

Previously, chrissank (Chris Sankey) wrote…

I agree. I just saw the document you linked, my only suggestion is that maybe a comment pointing to said explanation would be good, but as long as it's got an explanation somewhere, it's cool with me.

Yeah, maybe just worth commenting where the explanation can be found once it's ready

[shillcoat]

Reviewable status: all files reviewed, 21 unresolved discussions (waiting on @chrissank, @jacobdeery, and @wendi-yu)

a discussion (no related file):

Previously, wendi-yu (Wendi Yu) wrote…

Yes sorry I was even later lol, looks really good overall though :0

No worries, thanks guys!

---

---

configReader.py, line 11 at r3 (raw file):

Previously, wendi-yu (Wendi Yu) wrote…

Having the setting names hardcoded into here seems pretty easy to break accidentally, but I'm not sure if there's a better way to do it.

That's true, I went with this for the moment because that's how it was done previously so it was the easiest to re-implement. While it would probably be a good idea to eventually make this a bit more fool-proof, I don't have any ideas right now so I thought for the moment I would focus on getting everything running. Let me know what you think though.

---

constants.py, line 6 at r3 (raw file):

Previously, jacobdeery (Jacob Deery) wrote…

# Multiplying factor to convert from psi to Pascals

Comment isn't necessary, variable name is self-explanatory

Done.

---

databaseClass.py, line 21 at r3 (raw file):

Previously, jacobdeery (Jacob Deery) wrote…

object

No need to explicitly inherit from object; this can be just class DataBase:

Done.

---

databaseClass.py, line 38 at r3 (raw file):

Previously, jacobdeery (Jacob Deery) wrote…

Is there any reason to not immediately call this function when the database is initialized?

Nope, good point, I've fixed it so it just does it at initialization

---

databaseClass.py, line 42 at r3 (raw file):

Previously, jacobdeery (Jacob Deery) wrote…

        for row in csv.reader(open(self.path, 'r'), delimiter='\t'):
            if rowCount > -1:

Right now you open the file but never close it. This probably won't cause any real problems in this case, but it's best to avoid that. If you use a context manager (with open(self.path, 'r') as csvfile), the file will automatically close once it leaves scope.

Done.

---

databaseClass.py, line 78 at r3 (raw file):

Previously, jacobdeery (Jacob Deery) wrote…

if (T == 0 or T) and T <= 0:

prefer if (T is not None) and (T <=0); a bit more clear what the test is

Done.

---

databaseClass.py, line 82 at r3 (raw file):

Previously, jacobdeery (Jacob Deery) wrote…

same as above

Done.

---

databaseClass.py, line 91 at r3 (raw file):

Previously, jacobdeery (Jacob Deery) wrote…

This is cute

Cute in a good way or a bad way?

---

databaseClass.py, line 108 at r3 (raw file):

Previously, jacobdeery (Jacob Deery) wrote…

What does state represent?

Fantastic question, the answer to which is I have no clue. I put it in there without much thought because it was in the original code, but a quick look shows that it doesn't actually seem to be used anywhere. I've removed it.

---

getRPA.py, line 66 at r3 (raw file):

Previously, wendi-yu (Wendi Yu) wrote…

if name == ""? Unless I'm missing something lol

Lol nope, I just overcomplicated it. I'm getting rid of this entire section anyway, I found a better way to do this.

---

getRPA.py, line 69 at r3 (raw file):

Previously, jacobdeery (Jacob Deery) wrote…

"rpaWrapper\\" + name

os.path.join("rpaWrapper", name) works on all platforms and lets you avoid the manual escaping

Good point and this actually helped me out for work because I didn't know about this before, but I'm getting rid of this entire section in the end in any case.

---

getRPA.py, line 78 at r3 (raw file):

Previously, wendi-yu (Wendi Yu) wrote…

Format strings and triple quotes might make this easier to format, something like

confFile.write(f"""\
version = 1.2;
name = {name};
info = This file was generated automatically by OpenThrust;
""")

I didn't realize that's how that worked in python, cool. I'm not creating a new file in the end (this section is getting thrown out) but thanks!

---

getRPA.py, line 97 at r3 (raw file):

Previously, jacobdeery (Jacob Deery) wrote…

object

Not necessary

Done.

---

getRPA.py, line 106 at r3 (raw file):

Previously, jacobdeery (Jacob Deery) wrote…

Remove commented line? or is this necessary

Not necessary, I just tend to comment out what I'm not using anymore because I don't like deleting things. It's gone now.

---

main.py, line 4 at r3 (raw file):

Previously, jacobdeery (Jacob Deery) wrote…

This line errors for me because the first argument isn't a c_char_p

I don't know why I didn't see that earlier, but I've encoded the string first now so it should be alright. I also used os.path.join so I'm not doing the janky way of constructing the path myself.

---

requirements.txt, line 1 at r3 (raw file):

Previously, jacobdeery (Jacob Deery) wrote…

dataclasses is part of the standard library, so no need to add it here (but you should add scipy)

Done.

---

Vidar3RPA.cfg, line 1 at r3 (raw file):

Previously, jacobdeery (Jacob Deery) wrote…

Is this file actually for Vidar 3?

Yes, from the old repository I think. If not that, then from the google drive. I'm just using it to have a properly constructed config file to work with, when I move on to getting OpenThrust working with SOTS/KOTS I'll make a more relevant config file

---

Models/hybridModels.py, line 50 at r3 (raw file):

Previously, wendi-yu (Wendi Yu) wrote…

Is the expected failure mode for this just that configReader.ReadSettings() didn't successfully read in a value for self.V, or is there something else? Maybe worth mentioning in a comment

This is for catching if self.V was set to 0. I've added a comment in.

---

Models/injectorModel.py, line 35 at r2 (raw file):

Previously, wendi-yu (Wendi Yu) wrote…

Yeah, maybe just worth commenting where the explanation can be found once it's ready

Done.

---

Models/injectorModel.py, line 21 at r3 (raw file):

Previously, jacobdeery (Jacob Deery) wrote…

This condition might be a bit easier to read as: if self.parent.P1 < self.parent.P2 or self.parent.Pv1 <= self.parent.P2

Done. That's a much better way of doing it lol

---

UnitTests/test_configReader.py, line 3 at r3 (raw file):

Previously, jacobdeery (Jacob Deery) wrote…

import pytest
import configReader
from dataclasses import dataclass

from dataclasses import dataclass

import pytest

import configReader

Done.

---

UnitTests/test_configReader.py, line 24 at r3 (raw file):

Previously, jacobdeery (Jacob Deery) wrote…

pytest.approx

this is very cool, I was not aware it existed

Yep, I was so happy when I realized it did

---

UnitTests/test_databaseClass.py, line 3 at r3 (raw file):

Previously, jacobdeery (Jacob Deery) wrote…

import pytest
import databaseClass as db
from constants import NIST_PATH

same as previous comments

Done.

---

UnitTests/test_databaseClass.py, line 5 at r3 (raw file):

Previously, jacobdeery (Jacob Deery) wrote…

There's an extra newline here

Done.

---

UnitTests/test_databaseClass.py, line 13 at r3 (raw file):

Previously, jacobdeery (Jacob Deery) wrote…

Nicely done splitting the tests into separate classes. This is something I should probably do for topside.

Yay something I did right. It's an absolute pain

---

UnitTests/test_injectorModel.py, line 3 at r3 (raw file):

Previously, jacobdeery (Jacob Deery) wrote…

from Models.injectorModel import *
from dataclasses import dataclass
import pytest

same

Done.

[jacobdeery]

Great! Just a few minor comments left from me.

Reviewable status: 7 of 18 files reviewed, 6 unresolved discussions (waiting on @chrissank, @shillcoat, and @wendi-yu)

---

databaseClass.py, line 38 at r3 (raw file):

Previously, shillcoat (Sophie) wrote…

Nope, good point, I've fixed it so it just does it at initialization

I believe you should now be able to get rid of the loaded class member

---

databaseClass.py, line 91 at r3 (raw file):

Previously, shillcoat (Sophie) wrote…

Cute in a good way or a bad way?

In a good way. I like the propSet lambda.

---

Vidar3RPA.cfg, line 1 at r3 (raw file):

Previously, shillcoat (Sophie) wrote…

Yes, from the old repository I think. If not that, then from the google drive. I'm just using it to have a properly constructed config file to work with, when I move on to getting OpenThrust working with SOTS/KOTS I'll make a more relevant config file

Interesting. I suspect that this was a WIP config file that was intended to become Vidar 3 but never got all of the way there; CC pressure for the Vidar series was closer to 400 psi and we never used PAPI-94 in the fuel. No action required here, I was just curious.

---

UnitTests/test_databaseClass.py, line 3 at r3 (raw file):

Previously, shillcoat (Sophie) wrote…

Done.

sorry for not being clear; this should be

import pytest

import databaseClass as db
from constants import NIST_PATH

---

UnitTests/test_injectorModel.py, line 3 at r3 (raw file):

Previously, shillcoat (Sophie) wrote…

Done.

order should be dataclasses -> pytest -> injectorModel (standard library, then third-party, then local)

[shillcoat]

Reviewable status: 7 of 18 files reviewed, 6 unresolved discussions (waiting on @chrissank, @jacobdeery, and @wendi-yu)

---

databaseClass.py, line 38 at r3 (raw file):

Previously, jacobdeery (Jacob Deery) wrote…

I believe you should now be able to get rid of the loaded class member

Good point

---

databaseClass.py, line 91 at r3 (raw file):

Previously, jacobdeery (Jacob Deery) wrote…

In a good way. I like the propSet lambda.

Lol cool, thanks then.

---

UnitTests/test_databaseClass.py, line 3 at r3 (raw file):

Previously, jacobdeery (Jacob Deery) wrote…

sorry for not being clear; this should be

import pytest

import databaseClass as db
from constants import NIST_PATH

Whoops, so sorry. Done now.

---

UnitTests/test_injectorModel.py, line 3 at r3 (raw file):

Previously, jacobdeery (Jacob Deery) wrote…

order should be dataclasses -> pytest -> injectorModel (standard library, then third-party, then local)

Done. Sorry about that.

[jacobdeery]

!

Reviewable status: 7 of 18 files reviewed, 3 unresolved discussions (waiting on @chrissank and @wendi-yu)