RPA Extraction and Parsing

[shillcoat]

The file sampleRPAoutput.txt is just a sample file used for reference when parsing that I though would be useful to have going forward, it doesn't need to be reviewed (automatically generated by the wrapper).

---

This change is 

[BluCodeGH]

I can't comment on the correctness of the code, but in terms of style overall looks pretty good.

Reviewed 8 of 8 files at r1.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @shillcoat and @wendi-yu)

---

configReader.py, line 49 at r1 (raw file):

    RPAObject.fuelNames = []
    for fuel in config["Rocket Properties"]["fuel label(s)"].split(','):
        RPAObject.fuelNames += [fuel.strip()]

Using .append would be a bit cleaner here (and on similar lines below).

---

getRPA.py, line 89 at r1 (raw file):

# -----------------------------------------------

def parseRPA(outputData):

In general it's going to be hard to maintain this type of parsing where you have a bunch of flags that to keep track of where you are in the file. I've found an effective way to handle parsing like this is to use while line < len(outputData) as your loop. That lets you parse multiple lines at a time as needed (increment line more than one at a time) and you can manage the rest of the state via nested loops. Whether you want to rewrite this to work that way really depends on the future plans for the project, but I'd recommend it if you think there are going to need to be changes to the parsing here in the future. If you decide to keep the current method, please add comments.

---

getRPA.py, line 101 at r1 (raw file):

    units = False
    separationCount = 0
    for line in range(len(outputData)):

Since you don't use line outside of outputData[line], for line in outputData would be cleaner here.

---

RPA_TestScript.py, line 20 at r1 (raw file):

expectedKeys = ["Initial data", "Combustion chamber", "Combustion chamber end", "Throat", "Nozzle section"]
assert(len(rpaDict) == 5)
assert any(key in rpaDict.keys() for key in expectedKeys)

Don't you want to assert that all of the expected keys are found?

---

utils.py, line 6 at r1 (raw file):

class GrabOutput:

This looks right comparing against stack overflow but is pretty hard to understand, some comments would do nicely here.

[wendi-yu]

Same general gist here, mostly style and documentation comments from me. I think overall it would be helpful to comment the high level view of what the classes/functions each do, but the code looks good overall 💯

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

---

.gitignore, line 20 at r1 (raw file):

# Log files
*.log

missing final newline here :0

---

configReader.py, line 31 at r1 (raw file):

    # Set CC conditions
    RPAObject.ccPressure = float(config["Rocket Properties"]["chamber pressure (psi)"])

Since "Rocket Properties" and "Rocket Dimensions" are used a couple of times, it could be nice to pull them out into constants. If the name ever changes we only need to change the code in one place, and makes typos less likely for future development :0

---

getRPA.py, line 89 at r1 (raw file):

Previously, BluCodeGH (BluCode) wrote…

In general it's going to be hard to maintain this type of parsing where you have a bunch of flags that to keep track of where you are in the file. I've found an effective way to handle parsing like this is to use while line < len(outputData) as your loop. That lets you parse multiple lines at a time as needed (increment line more than one at a time) and you can manage the rest of the state via nested loops. Whether you want to rewrite this to work that way really depends on the future plans for the project, but I'd recommend it if you think there are going to need to be changes to the parsing here in the future. If you decide to keep the current method, please add comments.

Yeah, just to add on to the commenting, it would be good to add docstrings as well that describe what the function does and also describes the expected format of the input/output. Here's an example :0

[shillcoat]

I've tried to comment explaining all the main things (sorry, I'm not very good about doing that while I code). I don't think its really necessary to change the parser for the reasons I wrote below, though I do know it's a pretty quick and dirty solution.

Reviewable status: 2 of 8 files reviewed, 4 unresolved discussions (waiting on @BluCodeGH)

---

getRPA.py, line 89 at r1 (raw file):

Previously, wendi-yu (Wendi Yu) wrote…

Yeah, just to add on to the commenting, it would be good to add docstrings as well that describe what the function does and also describes the expected format of the input/output. Here's an example :0

I've added comments. I don't expect this output to ever change in format so I think its fine. The only situation I could foresee is getting the paid version of RPA, but that would make this entire section is redundant anyway.

---

getRPA.py, line 101 at r1 (raw file):

Previously, BluCodeGH (BluCode) wrote…

Since you don't use line outside of outputData[line], for line in outputData would be cleaner here.

Done.

---

RPA_TestScript.py, line 20 at r1 (raw file):

Previously, BluCodeGH (BluCode) wrote…

Don't you want to assert that all of the expected keys are found?

Good point. Done.

---

utils.py, line 6 at r1 (raw file):

Previously, BluCodeGH (BluCode) wrote…

This looks right comparing against stack overflow but is pretty hard to understand, some comments would do nicely here.

Done.

[BluCodeGH]

Reviewed 6 of 6 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @shillcoat)

---

getRPA.py, line 89 at r1 (raw file):

Previously, shillcoat (Sophie) wrote…

I've added comments. I don't expect this output to ever change in format so I think its fine. The only situation I could foresee is getting the paid version of RPA, but that would make this entire section is redundant anyway.

Alright sounds good 👍

---

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

        readRPAConf(self, confPath)

        #

Missing comment?