-
Notifications
You must be signed in to change notification settings - Fork 62
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Costing docs- initial template rollout #1361
Conversation
with open("detailed_unit_model_costing.rst", "a") as f: | ||
f.write(f" {unit_name_list[i]}\n") | ||
|
||
with open(f"{unit_name_list[i]}.rst", "w", encoding="utf-8") as f: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we can reuse this when more models are added, I think this should check for the existence of f"{unit_name_list[i]}.rst" before opening this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As in- "do not overwrite the file if it exists already"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or simply "only open and write to the file if it has been created already"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As in- "do not overwrite the file if it exists already"?
This one -- I assume these will be filled-out in subsequent PRs, and we don't want to overwrite files which already exist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was originally contemplating whether to automatically fill the tables with model params/vars as was done previously for the ZO model costing documentation. However, I am thinking we might as well just let each assignee manually fill out the relevant sections without needing to run this script (at least for the time being.
I can add a boolean option to enable/disable overwriting here, but unsure how necessary it would be if we turn the guidance into--don't run the script--just manually edit your rst file as needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, we'll want you're suggestion for later on, when new unit model methods get added. So I will try to add it in this PR.
@hunterbarber - I used the same structure that you used for GAC costing documentation, except I added a separate section for Costing Parameters (and Costing Variables). Check it out and let me know if you have thoughts on why it should stay as one section, as you had it. |
@@ -0,0 +1,27 @@ | |||
name,unit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could probably get rid of this csv and just read filenames from costing/unit_models, but isn't necessary to make that change yet.
docs/technical_reference/costing/automate_detailed_unit_costing_rst_file.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. One minor comment: should we include a link to the unit model documentation in the unit model costing method page? I think we had intended to include a link to the unit model costing method page in the unit model documentation, so doing the inverse on top of that might be redundant. Although doing so couldn't hurt...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple comments. After seeing responses I'll come back and approve.
docs/technical_reference/costing/automate_detailed_unit_costing_rst_file.py
Outdated
Show resolved
Hide resolved
f.write("\n\nReferences\n") | ||
f.write("-" * len("References")) | ||
f.write( | ||
"\nAim to include at least one reference in most cases, but delete this section if no references used for cost relationships/default values" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A disclaimer that the default values of parameters (say unit cost) correspond to the dollar value of the year of the reference (e.g. USD2020). Not necessarily saying this should go in the references, but if supplying units of default parameters should $ be listed as USDYYYY in all of these docs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps a link to other docs on costing, wherever currency units are explained, with a brief description on the fact that we can resolve variations in defaults (or provided values) with base_currency
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything would be converted to base flowsheet currency when solving, so that link probably should exist at a higher level documentation like the watertap_costing_package
just so it is not unnecessarily duplicated in every unit model. For these docs then, are you agreeing that the parameter tables should be completed using USD_YYYY instead of $ for the currency units?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, if reporting default parameters, makes sense to report the currency year.
) | ||
|
||
# TODO: add module directives to unit and cost method | ||
f.write("\nCode Documentation\n") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm assuming the author can should just include both the 0D and 1D models here, or other instances where unit_model:cost_model aren't 1:1. Say the some units having more dependencies on rectifier, heat exchanger, pumps, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because of that, I don't know if it makes sense to have once section as the model directly using the method/class and an optional section for other costing dependencies. Just a thought.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So instead of including any link to unit models here, I can just include the cost method, and later, within unit model tech refs, we can point to the relevant default costing method for the unit at hand. For example, in the tech ref for GAC, under costing section, We would just have a sentence pointing to the default costing method used and its tech ref (or something like that).
docs/technical_reference/costing/automate_detailed_unit_costing_rst_file.py
Show resolved
Hide resolved
…g_rst_file.py Co-authored-by: MarcusHolly <96305519+MarcusHolly@users.noreply.github.com>
Fixes/Resolves:
Partially addresses #1360
Summary/Motivation:
Initiates our effort to document costing of unit models by creating a landing page and template for all unit models currently in the
costing/unit_models
directory.After this PR, assignees should manually fill out their unit model costing rst files.
Changes proposed in this PR:
costing.unit_models
Legal Acknowledgement
By contributing to this software project, I agree to the following terms and conditions for my contribution: