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

Adding Save/Load Template to Routine Page #145

Merged
merged 27 commits into from
Feb 27, 2025
Merged

Conversation

michaellans
Copy link
Contributor

Introduces new features to routine page, with buttons to load and save templates for optimization runs.

  • Added "Load Template" button to env_cbox.py
  • Added "Save Template" button routine_page.py
  • New functions added to routine_page.py:
    • load_template_yaml
    • set_options_from_template
    • generate_template_dict_from_gui
    • save_template_yaml
  • Added signals to display save/load file message in status bar

michaellans and others added 25 commits January 16, 2025 16:24
Copy link
Collaborator

@shamin-slac shamin-slac left a comment

Choose a reason for hiding this comment

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

Overall looks good to me. Just had a few comments.

Also, I don't think it should be a problem, but you might want to merge in the more recent commits from main.

@@ -98,6 +98,16 @@ def init_ui(self):
vbox = QVBoxLayout(self)
vbox.setContentsMargins(8, 8, 8, 8)

# Load Template Button
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm guessing the button definitions are in different places because the actual buttons themselves are as well? Maybe you can show us tomorrow.

@@ -98,6 +100,10 @@ def __init__(self):
# Trigger the re-rendering of the environment box
self.env_box.relative_to_curr.setChecked(True)

# Template path
# template_dir = os.path.join(self.BADGER_PLUGIN_ROOT, "templates")
self.template_dir = "/home/physics/mlans/workspace/badger_test/Badger/src/badger/built_in_plugins/templates"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think what you have commented is the right idea.

@wenatuhs Do we want to keep it like this, or do you want to add a templates directory to your copy of Badger on prod?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree that we should go the commented way (template_dir = os.path.join(self.BADGER_PLUGIN_ROOT, "templates")), since templates are specific to the applications, so do the plugins, putting templates under plugins feels like the intuitive way.

@@ -168,7 +186,9 @@ def init_ui(self):
# vbox.addWidget(group_meta)

# Env box
BADGER_PLUGIN_ROOT = config_singleton.read_value("BADGER_PLUGIN_ROOT")
self.BADGER_PLUGIN_ROOT = BADGER_PLUGIN_ROOT = config_singleton.read_value(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Honestly, you might even just stick this in the constructor since this isn't really a UI thing.

@wenatuhs
Copy link
Collaborator

wenatuhs commented Feb 26, 2025

Tried rebasing the PR onto the main branch and it worked nicely, a few more things to consider:

  • The default template name in the save template dialog should be the one in the name field of the metadata tab, currently it's "templates"
  • The save/load template buttons feel a bit out of place due to the default width and height (in Badger we usually do 128 x 24 or 96 x 24)
  • Maybe we can use a combo box (as we do for the env selector) instead of a button for the load template feature, given that we'll use the fixed template folder (so we can read all templates beforehand)

All of the above can wait for the next PR though :)

@michaellans michaellans marked this pull request as ready for review February 27, 2025 00:08
Copy link
Collaborator

@wenatuhs wenatuhs left a comment

Choose a reason for hiding this comment

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

Thanks for the excellent work! I see no issues merging this into main :)

@wenatuhs wenatuhs merged commit d7f8dd7 into xopt-org:main Feb 27, 2025
5 checks passed
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.

3 participants