Skip to content

configure ruff - #260

Draft
njzjz wants to merge 12 commits into
deepmodeling:masterfrom
njzjz:ruff
Draft

configure ruff#260
njzjz wants to merge 12 commits into
deepmodeling:masterfrom
njzjz:ruff

Conversation

@njzjz

@njzjz njzjz commented Sep 10, 2024

Copy link
Copy Markdown
Member

No description provided.

njzjz and others added 4 commits September 10, 2024 17:32
Signed-off-by: Jinzhe Zeng <jinzhe.zeng@rutgers.edu>
Signed-off-by: Jinzhe Zeng <jinzhe.zeng@rutgers.edu>
@njzjz
njzjz marked this pull request as draft September 10, 2024 21:40
Comment thread dpgen2/entrypoint/main.py Outdated
Comment thread dpgen2/entrypoint/showkey.py
Comment thread dpgen2/entrypoint/status.py
Comment thread dpgen2/entrypoint/submit.py
Comment thread dpgen2/utils/artifact_uri.py
Comment thread dpgen2/utils/artifact_uri.py
Comment thread dpgen2/exploration/task/task_group.py
Comment thread dpgen2/exploration/task/lmp/lmp_input.py
Signed-off-by: Jinzhe Zeng <jinzhe.zeng@rutgers.edu>
Signed-off-by: Jinzhe Zeng <jinzhe.zeng@rutgers.edu>
Signed-off-by: Jinzhe Zeng <jinzhe.zeng@rutgers.edu>
Signed-off-by: Jinzhe Zeng <jinzhe.zeng@rutgers.edu>
Signed-off-by: Jinzhe Zeng <jinzhe.zeng@rutgers.edu>
@njzjz

njzjz commented Sep 10, 2024

Copy link
Copy Markdown
Member Author
/home/runner/work/dpgen2/dpgen2/dpgen2/flow/dpgen_loop.py:446:13 - error: Argument of type "bool" cannot be assigned to parameter "_if" of type "str | ArgoVar" in function "if_expression"

Why is the type of _if str | ArgoVar?

Signed-off-by: Jinzhe Zeng <jinzhe.zeng@rutgers.edu>
Signed-off-by: Jinzhe Zeng <jinzhe.zeng@rutgers.edu>
Comment thread dpgen2/flow/dpgen_loop.py
"exploration_scheduler"
].value_from_expression = if_expression(
_if=(scheduler_step.outputs.parameters["converged"] == True),
_if=(scheduler_step.outputs.parameters["converged"] is True),

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 — Preserve dflow’s overloaded equality expression. This value is a dflow OutputParameter, not a Python boolean. is True evaluates immediately to the literal Python value False, so all three if_expression calls always select the recursive _else branch. When the scheduler has converged, that branch references next_step even though its when=... == false condition skipped it, leaving workflow outputs unresolved. Restore the dflow comparison at lines 446, 451, and 456 and suppress E712 locally; the comparison is expression construction.

Suggested change
_if=(scheduler_step.outputs.parameters["converged"] is True),
_if=(scheduler_step.outputs.parameters["converged"] == True), # noqa: E712

Codex quota is about to reset, so I am using the remaining token budget to review this PR now.

Coding agent: Codex
Codex version: codex-cli 0.144.6
Model: gpt-5.6-sol
Reasoning effort: xhigh

Comment on lines +22 to +28
__all__ = [
"ConfFilter",
"ConfFilters",
"ConfSelector",
"ConfSelectorFrames",
"conf_filter_styles",
]

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 — Keep the concrete filter classes in wildcard exports. Adding __all__ changes the package API, but this list omits three classes that are still imported above. Before this PR, from dpgen2.exploration.selector import * exposed them; afterward they silently disappear.

Suggested change
__all__ = [
"ConfFilter",
"ConfFilters",
"ConfSelector",
"ConfSelectorFrames",
"conf_filter_styles",
]
__all__ = [
"BoxLengthFilter",
"BoxSkewnessConfFilter",
"ConfFilter",
"ConfFilters",
"ConfSelector",
"ConfSelectorFrames",
"DistanceConfFilter",
"conf_filter_styles",
]

Codex quota is about to reset, so I am using the remaining token budget to review this PR now.

Coding agent: Codex
Codex version: codex-cli 0.144.6
Model: gpt-5.6-sol
Reasoning effort: xhigh



def _sample_sphere():
rng = np.random.default_rng()

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 — Preserve a seedable PKA direction. Constructing a fresh entropy-seeded generator inside _sample_sphere() disconnects this operation from np.random.seed(), and neither this helper nor make_lmp_input() accepts a seed/generator. Existing callers therefore lose their only way to reproduce the generated direction. Thread an explicit generator or seed through the API, or retain the legacy global RNG with a targeted # noqa: NPY002; add a deterministic regression test. A complete fix requires an API decision, so a local suggestion is not safe.

Codex quota is about to reset, so I am using the remaining token budget to review this PR now.

Coding agent: Codex
Codex version: codex-cli 0.144.6
Model: gpt-5.6-sol
Reasoning effort: xhigh

selected_traj = [
trajs[np.random.randint(3, len(trajs) - 1)] for _ in range(4)
]
rng = np.random.default_rng()

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 — Preserve reproducible CALYPSO frame selection. This fresh entropy-seeded generator likewise ignores np.random.seed(), while parse_traj() offers no seed or generator parameter. Callers can no longer reproduce which frames are selected. Accept an explicit generator/seed or retain the legacy seeded global call with a targeted Ruff exemption, and add a deterministic selection test.

Codex quota is about to reset, so I am using the remaining token budget to review this PR now.

Coding agent: Codex
Codex version: codex-cli 0.144.6
Model: gpt-5.6-sol
Reasoning effort: xhigh

@njzjz-bot njzjz-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Reviewed this njzjz-authored draft as requested. The Ruff cleanup has one workflow-breaking dflow identity comparison, one public-export regression, and two reproducibility regressions from unseeded local generators. The branch is also conflicting with current master (14 content conflicts) and has no check runs/statuses on its current head.

Codex quota is about to reset, so I am using the remaining token budget to review this PR now.

Coding agent: Codex
Codex version: codex-cli 0.144.6
Model: gpt-5.6-sol
Reasoning effort: xhigh

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