Skip to content
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

Initial Commit for Plotting Templates #1

Merged
merged 10 commits into from
May 27, 2021

Conversation

dylanjm
Copy link

@dylanjm dylanjm commented May 19, 2021


Pull Request Description

What issue does this change request address?

This is a PR that will later be merged onto HERON devel.

What are the significant changes in functionality due to this change request?

This PR adds the plotting outstream templates for the dispatch plots in HERON.


For Change Control Board: Change Request Review

The following review must be completed by an authorized member of the Change Control Board.

  • 1. Review all computer code.
  • 2. If any changes occur to the input syntax, there must be an accompanying change to the user manual and xsd schema. If the input syntax change deprecates existing input files, a conversion script needs to be added (see Conversion Scripts).
  • 3. Make sure the Python code and commenting standards are respected (camelBack, etc.) - See on the wiki for details.
  • 4. Automated Tests should pass.
  • 5. If significant functionality is added, there must be tests added to check this. Tests should cover all possible options. Multiple short tests are preferred over one large tes.
  • 6. If the change modifies or adds a requirement or a requirement based test case, the Change Control Board's Chair or designee also needs to approve the change. The requirements and the requirements test shall be in sync.
  • 7. The merge request must reference an issue. If the issue is closed, the issue close checklist shall be done.
  • 8. If an analytic test is changed/added, the the analytic documentation must be updated/added.
  • 9. If any test used as a basis for documentation examples have been changed, the associated documentation must be reviewed and assured the text matches the example.

@dylanjm dylanjm marked this pull request as draft May 19, 2021 14:55
@dylanjm
Copy link
Author

dylanjm commented May 19, 2021

@PaulTalbot-INL A few design questions for the plotting outstreams.

I've currently setup the dispatch_plots as a boolean value in the debug case info node. I imagine this will then have to write a <Plot> node to the inner? Will anything need to be added to outer?

@PaulTalbot-INL
Copy link
Owner

I think right now when debug mode is enabled we're changing the return from Inner to Outer to go from being the sweep values to the dispatch results instead, so the outer can do the plotting (and place it somewhere easy to find). Like you suggest, I think we have the driver write the plot Step into the Outer after the MultiRun.

@dylanjm dylanjm marked this pull request as ready for review May 20, 2021 17:50
@dylanjm
Copy link
Author

dylanjm commented May 27, 2021

@PaulTalbot-INL This PR is pretty much ready for a once over. There are probably a few more details that I will add, but I believe the main logic and documentation is written for the debug plotting.

@PaulTalbot-INL
Copy link
Owner

Okay, I'm starting into it.

Copy link
Owner

@PaulTalbot-INL PaulTalbot-INL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple things to at least consider before we merge it in

src/Cases.py Outdated Show resolved Hide resolved
src/DispatchPlot.py Outdated Show resolved Hide resolved
src/DispatchPlot.py Show resolved Hide resolved
src/DispatchPlot.py Outdated Show resolved Hide resolved
templates/template_driver.py Show resolved Hide resolved
@@ -15,6 +15,7 @@
<debug>
<inner_samples>2</inner_samples>
<macro_steps>2</macro_steps>
<dispatch_plot>True</dispatch_plot>
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should add to the debug test a check for the existence of the output png (we don't need to test the image contents itself imo)

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a debug test already? If not I can just add the output check when I add the test.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think there is a debug unit test but there is a debug integration test.

@dylanjm
Copy link
Author

dylanjm commented May 27, 2021

@PaulTalbot-INL here is a plot output debug_dispatch_0_10_0.png

image

@dylanjm
Copy link
Author

dylanjm commented May 27, 2021

I would like to change the color cycle so that we get different colors for each resource.

@PaulTalbot-INL
Copy link
Owner

Yeah, coordinating colors through identifying unique components would be good (resources are already grouped, right?).

We could also simplify the labels to be only the unit, right, since it's all dispatch, although maybe for readability e.g. Steamer Dispatch would be more human than just Steamer? Either way we probably don't need to reiterate the resource, right?

@PaulTalbot-INL
Copy link
Owner

Someday maybe we add user-defined units for the X and Y axes, but for now this gives enough indication, and they have the CSV with the data if they want to make a production-worthy plot.

@dylanjm
Copy link
Author

dylanjm commented May 27, 2021

@PaulTalbot-INL Yes, I think streamlining the labels will be best. I'll make that change. And I totally agree, something the user can specify for y-axis labels would be nice but for the debug it might be okay to leave it blank as just a visual indicator.

[./debug_plot]
type = NetCDF
output = 'Debug_Run_o/dispatch_id0_y10_c0.png'
gold_files = 'gold/dispatch_id0_y10_c0.png'
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the output needs a gold file, since it just checks generation, but I can see what this does later.

@PaulTalbot-INL
Copy link
Owner

Merging to development branch, which will have SQA checklists performed before merging to devel.

@PaulTalbot-INL PaulTalbot-INL merged commit 1333d9e into PaulTalbot-INL:debug_mode May 27, 2021
PaulTalbot-INL pushed a commit that referenced this pull request May 24, 2022
Added TODO comments where debug case handling could use some work
PaulTalbot-INL pushed a commit that referenced this pull request Aug 3, 2022
@dylanjm dylanjm deleted the debug_mode branch April 11, 2023 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants