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.

Sign up for GitHub

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

Jump to bottom

Add a GUI script to the fuel cycle example #192

Merged
merged 10 commits into from
Oct 30, 2024
Merged

Add a GUI script to the fuel cycle example #192

merged 10 commits into from
Oct 30, 2024

Conversation

ke7kto
Copy link
Collaborator

@ke7kto ke7kto commented Oct 15, 2024 •
edited by cticenhour
Loading

Closes #191, also terminates the example if the tritium in storage ever reaches zero.

@ke7kto
Closes idaholab#191, adds a GUI script to the fuel cycle example, als…
db27684 
…o terminates the example if the tritium in storage ever reaches zero
@ke7kto
Copy link
Collaborator Author

ke7kto commented Oct 15, 2024

I wasn't sure the right syntax to point to the scripts directory, where I threw the GUI, from the documentation. That may need to be fixed.

@moosebuild
Copy link

moosebuild commented Oct 15, 2024 •
edited
Loading

Job Documentation, step Sync to remote on fcb6f11 wanted to post the following:

View the site here

This comment will be updated on new commits.

@moosebuild
Copy link

moosebuild commented Oct 15, 2024 •
edited
Loading

Job Coverage, step Generate coverage on fcb6f11 wanted to post the following:

Coverage

Coverage did not change

Full coverage report

This comment will be updated on new commits.

cticenhour
cticenhour requested changes Oct 22, 2024
View reviewed changes
Copy link
Member

@cticenhour cticenhour left a comment

Choose a reason for hiding this comment

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

Thank you for adding a new way to utilize the fuel cycle example! I have a few requests based on my initial review.

doc/content/examples/fuel_cycle/index.md Outdated Show resolved Hide resolved
scripts/fuel_cycle_gui.py Outdated Show resolved Hide resolved
scripts/fuel_cycle_gui.py Outdated Show resolved Hide resolved
scripts/fuel_cycle_gui.py Show resolved Hide resolved
@cticenhour cticenhour changed the title Closes #191, adds a GUI script to the fuel cycle example Add a GUI script to the fuel cycle example Oct 22, 2024
@cticenhour
Copy link
Member

I see we've got the script tested now - great!

I did show this to PC late last week and noticed that it doesn't handle differing screen resolutions well. It was looking great on my ultrawide display at work, but on my local MacBook display or on our conference room display, it was cutting off the lower parameter adjustment fields, the run buttons, the bottom portion of the rendered plot, as well as the figure view adjustment buttons. I then wasn't able to re-size or refresh this after the window completed rendering for the first time.

How easy or difficult would this be to fixup/add and make this more flexible for more systems?

@ke7kto
Copy link
Collaborator Author

ke7kto commented Oct 28, 2024 •
edited
Loading

I've made some changes to address the resizing issue. I don't think we'll be able to get a truly modern interface for this as long as we're using tkinter, but if we need to move to an HTML/CSS based GUI that'll be a weekend project.

cticenhour
cticenhour requested changes Oct 30, 2024
View reviewed changes
Copy link
Member

@cticenhour cticenhour left a comment

Choose a reason for hiding this comment

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

Tested window resizing - works much better on my laptop display! Thanks for fixing that. I think we're good with the TK-based UI for now. We can re-evaluate based on user feedback in the future.

Only a couple more adjustments I would like to make, and then I think this is ready to be merged.

doc/content/examples/fuel_cycle/index.md Show resolved Hide resolved
scripts/fuel_cycle_gui.py Show resolved Hide resolved
test/tests/fuel-cycle/tests Outdated Show resolved Hide resolved
ke7kto and others added 2 commits October 30, 2024 10:10
cticenhour
cticenhour approved these changes Oct 30, 2024
View reviewed changes
Copy link
Member

@cticenhour cticenhour left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thank you!

@cticenhour cticenhour merged commit 918e446 into idaholab:devel Oct 30, 2024
8 checks passed
@loganharbour
Copy link
Member

This broke moose next https://civet.inl.gov/job/2524190/

loganharbour
loganharbour reviewed Oct 31, 2024
View reviewed changes
test/tests/fuel-cycle/tests
@@ -7,4 +7,10 @@
csvdiff = 'fuel_cycle_out.csv'
requirement = 'The system shall reproduce a consistent solution to an ODE system of equations modeling the tritium fuel cycle.'
[]
[gui]
type = RunCommand
Copy link
Member

Choose a reason for hiding this comment

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

@cticenhour please reach out for guidance on these more complex tests. RunCommand is very versatile and also very dangerous when it comes to usability in installed binaries.

@loganharbour loganharbour mentioned this pull request Oct 31, 2024
Open
@loganharbour
Copy link
Member

Skipped for now in #208 until a determination is made on how to handle this with installs

@ke7kto ke7kto deleted the fuel_cycle_gui branch November 6, 2024 23:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@loganharbour loganharbour loganharbour left review comments

@cticenhour cticenhour cticenhour approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add a GUI script for the fuel cycle example
4 participants
@ke7kto @moosebuild @cticenhour @loganharbour