Add more documentation for loading reactors from database #75
Conversation
| @@ -67,6 +67,15 @@ | |||
| from .factory import databaseFactory | |||
|
|
|||
|
|
|||
| __all__ = [ | |||
There was a problem hiding this comment.
Might be good to add a comment describing why you need to redefine what the import * does for this module.
There was a problem hiding this comment.
It doesn't redefine, it defines what import * does for the package. So adding a comment to that effect would be duplicating the python docs for __all__
There was a problem hiding this comment.
Sure, I understand that, but there is some default behavior. from armi.bookkeping.db import * will do something whether or not you define all. This is why I used the word "redefine", but I don't want to split hairs on semantics.
I wasn't saying you should explain what this does. That is obvious, but rather why you are doing it. Why do we need special behavior here?
| @@ -455,9 +455,7 @@ | |||
| "metadata": {}, | |||
| "source": [ | |||
| "## Loading from the database\n", | |||
| "Once you have a database, you can load an operator object which has access to the reactor and case settings file.\n", | |||
| "\n", | |||
| "Warning: While the reactor is in the same state as it was when we wrote the database, the operator may not be. This is because ARMI uses the parameter system to load state, and only the reactor and its children have parameters. Furthermore, anything not stored in a paramter is not written to the database." | |||
There was a problem hiding this comment.
I feel like there was value in the statement that was removed about anything not stored as a parameter is not in the database, as this applies to block local variables, etc.
| @@ -8,6 +8,52 @@ the text of the input files that were used to create the case, and for each time | |||
| the values of all composite parameters as well as layout information to help fully | |||
| reconstruct the structure of the reactor model. | |||
|
|
|||
| Loading Reactor State | |||
|
|
||
| from armi.bookkeeping.db import databaseFactory | ||
|
|
||
| db = databaseFactory("myDatabase.h5", "r") |
There was a problem hiding this comment.
It might be worthwhile at some point to have a default of "r" to the permission argument (not in this diff). The vast majority of time people will call this in scripts, it will be "r". To get a reactor, needing databasePath, cycle, and timeNode is pretty elegant. Needed to remember "r" as well is less so.
There was a problem hiding this comment.
Optional arguments lead to ambiguity, which normally requires looking up the function docstring to resolve, so it would only be elegant to a person that is already intimately familiar with the API. Also, having the second argument lends parity all of the built-in open()-like functions throughout the python ecosystem.
There was a problem hiding this comment.
the built-in open() has a default of mode 'r' (just like I am suggesting)
https://docs.python.org/3/library/functions.html#open
I think making it a kwarg would lend more parity. Note also that open() had quite a few other kwargs that normal people don't use on a daily basis, but this doesn't mean I have to look up the doc strings constantly to resolve. I can't think of a time that I ever had a armi script where I want to load in the db in 'w' mode, but 'r' mode is extremely common.
No description provided.