Resourcegraph #223
Resourcegraph #223
Conversation
|
Here is what the plots look like:
|
| @@ -154,13 +168,14 @@ def main(): | |||
| sim.read_input(args.xml_input_file) # TODO expand to use arguments? | |||
| # print details | |||
| sim.print_me() | |||
| sim.plot_resource_graph() | |||
There was a problem hiding this comment.
This might be fine initially, but I think this network graph should be something we do at the HERON compilation stage instead of during dispatch optimization. How hard would it be to move?
There was a problem hiding this comment.
I believe this is creating the plot during HERON compilation. If debug mode is enabled it will create the plot when running heron heron_input.xml
There was a problem hiding this comment.
Oh, you're right, I'm sorry. I saw sim.print_me() and immediately thought of the pyomo model. I should have looked more closely! I'm still on travel but can review this tomorrow or Friday probably.
| in a HERON system. | ||
| """ | ||
|
|
||
| def __init__(self, components: list) -> None: |
There was a problem hiding this comment.
I see you added annotations to method definitions (-> and the like). It is PEP compliant (3107) and I think it adds some valuable information, especially to developers at a quick glance. I haven't seen these annotations elsewhere in HERON, should we standardize this practice going forward? What I mean is, should we include these annotations going forward or remove them? I would vote for including them.
There was a problem hiding this comment.
Yes, I agree. I like having the annotations and the more detailed we can be with them, the more easy our code would be to understand. Perhaps maybe a soft encouragement to use them going forward? We could easily add a pylint type checking test to our Precheck on CIVET if we wanted to make it a hard standard. @PaulTalbot-INL @joshua-cogliati-inl do you have any specific ideas or reservations about this?
There was a problem hiding this comment.
I am not ready to codify it as required practice, but I certainly am happy to see it added where possible. If forced into a decision, I would prefer to include them, but it will take time and opportunity to convert existing code to this standard. I would also like to re-think the RAVEN approach to docstrings at the same time, since this info is largely included in the type hints, but that represents a significant change for a lot of code, possibly.
There was a problem hiding this comment.
I am okay with optionally adding typing annotations. We should add checking them with mypy or pyright or similar before making them required.
|
@dgarrett622 are you available to finish the review of this PR? |
Pull Request Description
What issue does this change request address?
#163
What are the significant changes in functionality due to this change request?
This PR implements the ability to create a resource graph png during a debug mode run
For Change Control Board: Change Request Review
The following review must be completed by an authorized member of the Change Control Board.