Replace the function preparse to default_group_name. - #38
Merged
Conversation
hzhangxyz
requested review from
CuSO4Deposit,
Copilot and
stevapple
and removed request for
CuSO4Deposit and
Copilot
June 16, 2025 11:22
There was a problem hiding this comment.
Pull Request Overview
This PR renames the function from preparse to default_group_name and updates its argument type from tuple[str, ...] to ModelConfig. It also updates all corresponding usages and docstrings across multiple files to reflect the new function name and parameter.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| qmb/openfermion.py | Function renamed to default_group_name with updated argument. |
| qmb/model_dict.py | Updated function and docstring to reflect the new function naming. |
| qmb/ising.py | Renamed function with updated logic using ModelConfig and string building. |
| qmb/fcidump.py | Renamed function to default_group_name with updated argument usage. |
| qmb/common.py | Updated usage to retrieve ModelConfig via tyro.cli and pass it to default_group_name. |
Comment on lines
+75
to
+91
| def default_group_name(cls, config: ModelConfig) -> str: | ||
| # pylint: disable=too-many-locals | ||
| args = tyro.cli(ModelConfig, args=input_args) | ||
| x = f"_x{args.x}" if args.x != 0 else "" | ||
| y = f"_y{args.y}" if args.y != 0 else "" | ||
| z = f"_z{args.z}" if args.z != 0 else "" | ||
| xh = f"_xh{args.xh}" if args.xh != 0 else "" | ||
| yh = f"_yh{args.yh}" if args.yh != 0 else "" | ||
| zh = f"_zh{args.zh}" if args.zh != 0 else "" | ||
| xv = f"_xv{args.xv}" if args.xv != 0 else "" | ||
| yv = f"_yv{args.yv}" if args.yv != 0 else "" | ||
| zv = f"_zv{args.zv}" if args.zv != 0 else "" | ||
| xd = f"_xd{args.xd}" if args.xd != 0 else "" | ||
| yd = f"_yd{args.yd}" if args.yd != 0 else "" | ||
| zd = f"_zd{args.zd}" if args.zd != 0 else "" | ||
| xa = f"_xa{args.xa}" if args.xa != 0 else "" | ||
| ya = f"_ya{args.ya}" if args.ya != 0 else "" | ||
| za = f"_za{args.za}" if args.za != 0 else "" | ||
| x = f"_x{config.x}" if config.x != 0 else "" | ||
| y = f"_y{config.y}" if config.y != 0 else "" | ||
| z = f"_z{config.z}" if config.z != 0 else "" | ||
| xh = f"_xh{config.xh}" if config.xh != 0 else "" | ||
| yh = f"_yh{config.yh}" if config.yh != 0 else "" | ||
| zh = f"_zh{config.zh}" if config.zh != 0 else "" | ||
| xv = f"_xv{config.xv}" if config.xv != 0 else "" | ||
| yv = f"_yv{config.yv}" if config.yv != 0 else "" | ||
| zv = f"_zv{config.zv}" if config.zv != 0 else "" | ||
| xd = f"_xd{config.xd}" if config.xd != 0 else "" | ||
| yd = f"_yd{config.yd}" if config.yd != 0 else "" | ||
| zd = f"_zd{config.zd}" if config.zd != 0 else "" | ||
| xa = f"_xa{config.xa}" if config.xa != 0 else "" | ||
| ya = f"_ya{config.ya}" if config.ya != 0 else "" | ||
| za = f"_za{config.za}" if config.za != 0 else "" |
There was a problem hiding this comment.
[nitpick] The string-building logic for the group name is repetitive; consider extracting a helper function for creating these formatted segments to improve maintainability.
There was a problem hiding this comment.
纠结，我感觉现在这个写法确实不太好而且容易出错，但是抽成函数之后大概率需要 getattr 之类更不安全的操作，可读性可能也未必特别好。
stevapple
approved these changes
Jun 16, 2025
Comment on lines
+75
to
+91
| def default_group_name(cls, config: ModelConfig) -> str: | ||
| # pylint: disable=too-many-locals | ||
| args = tyro.cli(ModelConfig, args=input_args) | ||
| x = f"_x{args.x}" if args.x != 0 else "" | ||
| y = f"_y{args.y}" if args.y != 0 else "" | ||
| z = f"_z{args.z}" if args.z != 0 else "" | ||
| xh = f"_xh{args.xh}" if args.xh != 0 else "" | ||
| yh = f"_yh{args.yh}" if args.yh != 0 else "" | ||
| zh = f"_zh{args.zh}" if args.zh != 0 else "" | ||
| xv = f"_xv{args.xv}" if args.xv != 0 else "" | ||
| yv = f"_yv{args.yv}" if args.yv != 0 else "" | ||
| zv = f"_zv{args.zv}" if args.zv != 0 else "" | ||
| xd = f"_xd{args.xd}" if args.xd != 0 else "" | ||
| yd = f"_yd{args.yd}" if args.yd != 0 else "" | ||
| zd = f"_zd{args.zd}" if args.zd != 0 else "" | ||
| xa = f"_xa{args.xa}" if args.xa != 0 else "" | ||
| ya = f"_ya{args.ya}" if args.ya != 0 else "" | ||
| za = f"_za{args.za}" if args.za != 0 else "" | ||
| x = f"_x{config.x}" if config.x != 0 else "" | ||
| y = f"_y{config.y}" if config.y != 0 else "" | ||
| z = f"_z{config.z}" if config.z != 0 else "" | ||
| xh = f"_xh{config.xh}" if config.xh != 0 else "" | ||
| yh = f"_yh{config.yh}" if config.yh != 0 else "" | ||
| zh = f"_zh{config.zh}" if config.zh != 0 else "" | ||
| xv = f"_xv{config.xv}" if config.xv != 0 else "" | ||
| yv = f"_yv{config.yv}" if config.yv != 0 else "" | ||
| zv = f"_zv{config.zv}" if config.zv != 0 else "" | ||
| xd = f"_xd{config.xd}" if config.xd != 0 else "" | ||
| yd = f"_yd{config.yd}" if config.yd != 0 else "" | ||
| zd = f"_zd{config.zd}" if config.zd != 0 else "" | ||
| xa = f"_xa{config.xa}" if config.xa != 0 else "" | ||
| ya = f"_ya{config.ya}" if config.ya != 0 else "" | ||
| za = f"_za{config.za}" if config.za != 0 else "" |
There was a problem hiding this comment.
纠结，我感觉现在这个写法确实不太好而且容易出错，但是抽成函数之后大概率需要 getattr 之类更不安全的操作，可读性可能也未必特别好。
Member
Author
确实，只是不知道咋样好，因为希望把所有参数都显示出来，所以只好先这样了。 |
hzhangxyz
force-pushed
the
dev/rename-preparse-to-default-name
branch
from
June 16, 2025 15:18
79e3c99 to
c5ed0cd
Compare
hzhangxyz
force-pushed
the
dev/rename-preparse-to-default-name
branch
from
June 16, 2025 15:20
c5ed0cd to
c220c64
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
The previous function name
preparseis not proper, the only usage of it is to get the default group name, so change it to better function name, by the way, we also update the argument of it fromtuple[str, ...]toModelConfigas the process from tuple to config is common and we shall run it incommon.py.Checklist: