Refactor depcode#64
Conversation
gwenchee
left a comment
There was a problem hiding this comment.
Looks good. I have some comments about variable naming etc. Also, you seem to have some pep8 issues.
| ---------- | ||
| dep_file : str | ||
| Path to file containing results of depletion simulation | ||
| moment : int |
There was a problem hiding this comment.
This doesnt seem very intuitive. Why not a boolean with a variable name read_at_start
There was a problem hiding this comment.
I agree. moment makes me think of moment of force, stuff like that. I'll look into changing this.
LukeSeifert
left a comment
There was a problem hiding this comment.
Looks good! Everything seems to be working as well when I ran it with the msbr example case (had to change Andrei's cross section locations to my own and create an empty "data" directory in saltproc, but had to do that before anyways).
| Path to user template file for depletion code | ||
| inp : str | ||
| Path to input file for depletion code rerunning | ||
| reactor : Reactor |
There was a problem hiding this comment.
I don't think this data type is correct
| Contains information about power load curve and cumulative | ||
| depletion time for the integration test. | ||
| dts : int | ||
| dep_step : int |
There was a problem hiding this comment.
Nice change here, much more clear variable name than dts
|
@gwenchee @LukeSeifert @munkm I believe I've addressed all requested changes, so if there aren't any more issues could one of you approve so we can merge? |
gwenchee
left a comment
There was a problem hiding this comment.
You haven't addressed the moment comment we discussed above but besides that LGTM!
|
@gwenchee did you see my follow up comment about that?
I guess since we are only every using 0 or 1 as the value for |
This PR changes the structure of the
Depcodeclass to use python's Abstract Base Class to make the changes outlined in #59 .This PR also adjusts the import statements in the test suite to match the new class names.