-
Notifications
You must be signed in to change notification settings - Fork 57
Changes to definitions of named gauge optimization suites, resolve (unnecessary) warnings, robustify report generation #622
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
Conversation
…obust report generation). Remove an unnecessary warning.
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.
The branching was unnecessary, since jac=None
is the default value for jac
in both this function and the scipy function we're calling.
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.
The overwhelming majority of changes to this file are in _update_gaugeopt_dict_from_suitename
function of the GSTGaugeOptSuite class. Among these changes, only two have operational consequences:
- I changed it from an instance method to a static method, since "self" was never referenced.
- I changed a key-value pair in a dict literal that was conditionally assigned to a variable called "convert_to". The key-value pair used to be ("to_type", "full TP"), while it's now ("to_type", "full"). This has no effect on the gauge group that gets used for gauge optimization.
All other changes to _update_gaugeopt_dict_from_suitename amount to readability. Here are notes I made while convincing myself of this
Details for the morbidly curious
I'll start with notes confined entirely to _update_gaugeopt_dict_from_suitename.
If we enter the branch where convert_to gets defined at all, then we effectively return control to the calling function without leaving the indentation level at which convert_to is defined (i.e., there aren't subsequent branches after the
if
on line 990 that would use convert_to as defined on 995).Observations about gg := model.default_gauge_group
- If gg is None AND "noconversion" is not in suite_name, then line 996 will raise an attribute error when trying to read None.name. Therefore, all valid codepaths have either "noconversion" in suite_name OR gg is not None. However, if "noconversion" is in suite_name then convert_to is just set to None.
- If gg is None then the isinstance check at line 998 will fail, and the "is not None" check at line 1006 obviously fails, and so both the if and elif are skipped. Since there is no else clause, that means if gg is None then the function will return without actually modifying gaugeopt_suite_dict.
Summary of possibilities:
- gg is None. In this case, the function returns with no sideeffects visible from the calling function.
- gg is not None, but "noconversion" is in suite_name. In this case, the value of the dict literal that I'm modifying is irrelevant, because the dict literal is never assigned to convert_to.
- gg is not None, and "noconversion" is NOT in suite_name. In this case, the isinstance check at line 998 fails but the not-None check at line 1006 succeeds. This means the function basically reduces to lines 1009 to 1062. The convert_to variable occurs exactly twice in that portion of code. Both occurrences are in dicts with explicit gauge groups.
The dicts that get constructed in _update_gaugeopt_dict_from_suitename get passed as kwargs in call(s) to gaugeopt_to_target. So, understanding the consequences of my changes to _update_gaugeopt_dict_from_suitename amounts to understanding how gaugeopt_to_target behaves when provided with kwargs of the kind constructed in lines 1009 -- 1062 of gst.py.
When the convert_model_to kwarg is None, the change I made to the
convert_to
dict in _update_gaugeopt_dict_from_suitename is obviously irrelevant.When the convert_model_to kwarg is a dict, gaugeopt_to_target ends up passing the key-value pairs as kwargs in a call to model.convert_members_inplace. If convert_model_to has a key-value pair ("set_default_gaugegroup", True), this call to convert_members_inplace sets the model's default gauge group. HOWEVER, if we assume that gaugeopt_to_target's "gauge_group" kwarg is not None, which we CAN assume in the context of lines 1009--1062, then of gst.py, then the model's default_gauge_group member never ends up being accessed. (Verifying this requires stepping into the custom_gaugeopt function that gaugeopt_to_target ends up calling, but that verification is easy.)
Now let's look back at _update_gaugeopt_dict_from_suitename.
From the behavior outlined above, we see that a change to the to_type key of the convert_to dict has no effect on the gauge group used in gauge optimization. However, I've opted to also change convert_to's "set_default_gaugegroup" key to False, since that makes our intent clearer.
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.
Awesome work, thank you for tracking this down and for the detailed summary!
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.
Style and "hygiene."
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.
Changes accidentally left out of PR #615.
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.
This resolves a deprecation warning.
…ly asked for functionality that was removed around that time
…int to float to make type checker happy
… key in dicts by "k in d" rather than the redundant "k in d.keys()".
@coreyostrove this is ready for review |
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 looks good to me, thanks for the great work!
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.
Awesome work, thank you for tracking this down and for the detailed summary!
The main change in this PR is to take the following line(s) in GSTGaugeOptSuite._update_gaugeopt_suite_from_suitename
and replace
'to_type': "full TP"
with'to_type': "full"
. It's preferable to use "full" rather than "full TP" because the latter only works for some bases in Hilbert--Schmidt space.See individual file comments for explanation of changes to other files.