Skip to content

Elements: Consistent Defaults#1449

Open
ax3l wants to merge 4 commits into
BLAST-ImpactX:developmentfrom
ax3l:topic-elements-consistent-default
Open

Elements: Consistent Defaults#1449
ax3l wants to merge 4 commits into
BLAST-ImpactX:developmentfrom
ax3l:topic-elements-consistent-default

Conversation

@ax3l
Copy link
Copy Markdown
Member

@ax3l ax3l commented May 4, 2026

In our current workflow, we have to repeat ourselves quite a bit when it comes to defaults of element parameters:

  • C++ constructor: purely informational, not used anywhere unless someone uses ImpactX as C++ library and does not pass explicit values (internally, we do pass explicit values in init)
  • InitElements: the ParmParse/inputs defaults
  • elements.cpp: the Python defaults
  • parameters.rst: the ParmParse/inputs docs
  • python.rst: the Python docs

This PR simplifies this for the first three to a single, authoritative source: the C++ implementation of the element.

PR #699 will then also unify the python.rst file into the same authorative source, leaving only the parameters.rst to be manually (or LLM-y) kept right.

Result, less repetition: 5x same value -> 2x same value

DRY programming like this is desirable where possible (not a hard rule*), since repetition leads quickly inconsistencies over time (e.g., #1445).
(*Programming everything DRY can also lead to hard-to-extend, rigid code, so we strike a balance.)

Changed Defaults

Same as #1445, the ExactCFbend, ExactMultipole, ExactQuad now use 10 mapsteps as the default.

Old C++ default was 10, too (init elements). But:

  • C++ docs were off to 5.
  • Python & its docs where off to 5.

@ax3l ax3l requested a review from cemitch99 May 4, 2026 03:17
@ax3l ax3l added the component: elements Elements/maps/external fields label May 4, 2026
Comment thread src/elements/Buncher.H Outdated
Comment thread src/elements/Buncher.H Outdated
Comment thread src/elements/CFbend.H Outdated
Comment thread src/elements/CFbend.H
ax3l added 2 commits May 4, 2026 08:45
In our current workflow, we have to repeat ourselves quite a
bit when it comes to defaults of element parameters:
- C++ constructor: purely informational, not used anywhere
  unless someone uses ImpactX as C++ library and does not
  pass explicit values (internally, we do pass explicit values
  in init)
- InitElements: the ParmParse/inputs defaults
- elements.cpp: the Python defaults
- parameters.rst: the ParmParse/inputs docs
- python.rst: the Python docs

This PR simplifies this for the first three to a single, authorative
source: the C++ implementation of the element.
@ax3l ax3l force-pushed the topic-elements-consistent-default branch from 808be0f to bdafe10 Compare May 4, 2026 15:46
@ax3l ax3l requested a review from EZoni May 4, 2026 15:53
@ax3l ax3l force-pushed the topic-elements-consistent-default branch from bdafe10 to 3388252 Compare May 4, 2026 15:55
@ax3l ax3l force-pushed the topic-elements-consistent-default branch from 3388252 to fdd3ffd Compare May 4, 2026 16:24
@codspeed-hq
Copy link
Copy Markdown

codspeed-hq Bot commented May 4, 2026

Merging this PR will not alter performance

✅ 37 untouched benchmarks


Comparing ax3l:topic-elements-consistent-default (2cb9092) with development (ac4fe71)

Open in CodSpeed

Comment thread src/elements/ExactCFbend.H
@ax3l ax3l mentioned this pull request May 4, 2026
Comment thread src/elements/ExactCFbend.H Outdated
@ax3l ax3l added the changes input scripts / defaults Changes the syntax or meaning of input scripts and/or defaults label May 4, 2026
Comment thread src/elements/ExactMultipole.H Outdated
Old C++ default was 10, too (init elements).

C++ docs were off to 5.
Python & its docs where off to 5.
@ax3l ax3l requested a review from egstern May 7, 2026 03:41
Copy link
Copy Markdown
Member

@cemitch99 cemitch99 left a comment

Choose a reason for hiding this comment

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

I'm taking my time a bit with this one. I have a slight concern that this increases the complexity of the element code by introducing a large number of DEFAULT_ parameters, most of which are zero. Otherwise, I agree it is a good way to ensure consistency.

@ax3l
Copy link
Copy Markdown
Member Author

ax3l commented May 12, 2026

Yeah, I found the consistency to be worth the slight verbosity. Let me know where you arrive at @cemitch99

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changes input scripts / defaults Changes the syntax or meaning of input scripts and/or defaults component: elements Elements/maps/external fields

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants