Skip to content

[RL] Add periodic boundary conditions to all grid-based competition modules#403

Open
Lgz-tud wants to merge 5 commits into
masterfrom
feature/pbc-all-modules
Open

[RL] Add periodic boundary conditions to all grid-based competition modules#403
Lgz-tud wants to merge 5 commits into
masterfrom
feature/pbc-all-modules

Conversation

@Lgz-tud

@Lgz-tud Lgz-tud commented Apr 22, 2026

Copy link
Copy Markdown
Collaborator

Summary

Part of #401

  • Lift PBC to base class: ResourceModel.makeGrid() now stores domain size (_lx, _ly) and parses the optional <periodic_boundary> XML tag. New wrapDistance(dx, dy) method applies the minimum image convention.
  • Refactor FON: Remove FON's own PBC parsing, use inherited base class infrastructure instead.
  • Add PBC to AsymmetricZOI, SymmetricZOI, SaltFeedbackBucket: Each module accepts <periodic_boundary>True</periodic_boundary> in its XML config. Distance calculations are wrapped via wrapDistance().

Usage

Add to any grid-based resource module's XML config:

<periodic_boundary> True </periodic_boundary>

@Lgz-tud Lgz-tud requested a review from mcwimm April 22, 2026 10:03
@mcwimm

mcwimm commented May 8, 2026

Copy link
Copy Markdown
Collaborator

Very cool! Do you think it's possible to define the periodic boundary only once?

<resources>
    <periodic_boundary> True</periodic_boundary>
    <aboveground>
        <type> Default </type>
    </aboveground>
    <belowground>
        <type> Default </type>
    </belowground>
</resources>

Lgz-tud added a commit that referenced this pull request May 11, 2026
Read the optional <periodic_boundary> tag once at the <resources> level and
propagate it to both above- and below-ground module configs. Defining the
tag inside <aboveground> or <belowground> now raises a clear ValueError.

Updates docs (ResourceModel, AsymmetricZOI, SymmetricZOI, SaltFeedbackBucket)
and migrates the FON_PBC_CI benchmark XML.

Addresses review comment in PR #403.
@Lgz-tud

Lgz-tud commented May 11, 2026

Copy link
Copy Markdown
Collaborator Author

Thanks for the suggestion — agreed, defining it once at the level is much cleaner.

  • <periodic_boundary> is now read once at the level and propagated to both above- and below-ground module configs.
  • Defining the tag inside or raises a ValueError with a clear message pointing to the new location.
  • FON_PBC_CI benchmark XML migrated to the new layout (reference output unchanged).
  • Module docs (ResourceModel, AsymmetricZOI, SymmetricZOI, SaltFeedbackBucket) updated with the new example.

@mcwimm mcwimm left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Very cool 👍

"prj_file": args,
"required": ["type", "domain", "x_1", "x_2", "y_1", "y_2", "x_resolution", "y_resolution"],
"optional": ["allow_interpolation", "backend_type"],
"optional": ["allow_interpolation", "backend_type", "periodic_boundary"],

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Shouldn't "periodic_boundary" be removed from the tag dictionaries of all resource modules?

Lgz-tud added 4 commits June 6, 2026 21:16
Move PBC (periodic boundary conditions) support from FON into the shared
ResourceModel base class so all grid-based competition modules can reuse it.

Changes:
- ResourceModel.makeGrid() now stores domain size (_lx, _ly) and parses
  the optional <periodic_boundary> XML tag
- ResourceModel.wrapDistance() applies minimum image convention
- FON: refactored to use base class PBC infrastructure
- AsymmetricZOI, SymmetricZOI, SaltFeedbackBucket: added PBC support
  via the new wrapDistance() helper

All 53 existing benchmarks pass unchanged.
Update ResourceModel.md, AsymmetricZOI.md, SymmetricZOI.md, and
SaltFeedbackBucket.md with periodic_boundary attribute descriptions,
usage notes, and XML examples.
Read the optional <periodic_boundary> tag once at the <resources> level and
propagate it to both above- and below-ground module configs. Defining the
tag inside <aboveground> or <belowground> now raises a clear ValueError.

Updates docs (ResourceModel, AsymmetricZOI, SymmetricZOI, SaltFeedbackBucket)
and migrates the FON_PBC_CI benchmark XML.

Addresses review comment in PR #403.
Match the convention applied in PR #400 (FON.md). Linking to the
package path lets users jump directly to the generated documentation.
@Lgz-tud Lgz-tud force-pushed the feature/pbc-all-modules branch from 6afe065 to da1c2bb Compare June 6, 2026 19:22
Since periodic_boundary is now configured once at the <resources>
level and injected into every resource module's tag tree by
XMLtoProject, the per-module tag dictionaries should no longer
list it as an optional tag. Move the responsibility into the base
class ResourceModel.getInputParameters, which auto-appends
periodic_boundary to the optional list for any sub-class.

Addresses Marie's review comment on PR #403.
@Lgz-tud

Lgz-tud commented Jun 6, 2026

Copy link
Copy Markdown
Collaborator Author

Two changes since @mcwimm last review:

  1. Rebased onto master (which now includes [RL] FON: add optional C++ backend #400's C++ backend).

  2. Removed periodic_boundary from per-module tag dictionaries . Base class ResourceModel.getInputParameters auto-injects it now.

All benchmarks pass. Ready for re-review.

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