Skip to content

Conversation

@stewartboogert
Copy link
Collaborator

No description provided.

Copy link
Contributor

@lnevay lnevay left a comment

Choose a reason for hiding this comment

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

Various little comments. But work in progress 👍

@stewartboogert stewartboogert force-pushed the acceleration branch 3 times, most recently from 6bc6f37 to 7b13bfa Compare February 14, 2025 21:22
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should avoid committing notebooks as they inflate the repository size and are more personal. Python scripts fine.

{
public:
BDSFieldEMAcceleration();
~BDSFieldEMAcceleration();
Copy link
Contributor

Choose a reason for hiding this comment

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

Destructor should be marked virtual.

#include "BDSFieldEM.hh"

#include <vector>

Copy link
Contributor

Choose a reason for hiding this comment

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

Please add some basic doxygen just before the class declaration.

/** 
 * @brief  brief description
 *
 * @author Stewart Boogert
 */

class BDSFieldEMAxialStandingApprox : public BDSFieldEMAcceleration {
public:
BDSFieldEMAxialStandingApprox() = delete;

Copy link
Contributor

Choose a reason for hiding this comment

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

virtual ~BDSFieldEMAxisStandingApprox() = default;

class BDSFieldEMAxialFloquetApprox : public BDSFieldEMAcceleration {
public:
BDSFieldEMAxialFloquetApprox() = delete;

Copy link
Contributor

Choose a reason for hiding this comment

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

destructor

#include "BDSFieldEMAcceleration.hh"

class BDSMagnetStrength;

Copy link
Contributor

Choose a reason for hiding this comment

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

doxygen

@@ -0,0 +1,62 @@
//
// Created by Boogert Stewart on 01/12/2024.
Copy link
Contributor

Choose a reason for hiding this comment

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

Licence missing.

//

#include "BDSFieldEMAxlalFloquetApprox.hh"
#include "BDSMagnetStrength.hh"
Copy link
Contributor

Choose a reason for hiding this comment

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

#include "G4ThreeVector.hh"

#include <cmath>

@@ -0,0 +1,124 @@
//
// Created by Boogert Stewart on 25/11/2024.
Copy link
Contributor

Choose a reason for hiding this comment

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

Licence. Authorship goes in header in doxygen.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Defo. I'll stick it in as we have it now But we should really rewrite the header to reflect contributions from all. Copyright Royal Holloway is not totally inaccurate and not great for you, me, Fabian, Giacommo, etc. I'll raise an issue

@lnevay
Copy link
Contributor

lnevay commented Feb 19, 2025

Oh, noticed we're missing the description in the manual also. source/dev_fields.rst please.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR extends the accelerator module by adding three new RF field types—transversemagnetic, axialstandingapprox, and axialfloquetapprox—along with their data keys, unit factors, integrator support, factory registration, and class implementations. It also fixes the half-wavelength calculation for cell length and updates related enum dictionaries.

  • Added new cavity parameter keys and unit factors in BDSMagnetStrength.
  • Introduced BDSFieldEMCircularTM, BDSFieldEMAxialStandingApprox, and BDSFieldEMAxialFloquetApprox classes with factory wiring.
  • Updated integrator set, field/type mappings, and component factories; corrected cellLength = c_light/(2*frequency).

Reviewed Changes

Copilot reviewed 24 out of 24 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
src/BDSMagnetStrength.cc Extended strength keys/unit factors and removed TODOs
src/BDSIntegratorSet.cc Added integrator fields and missing cases
src/BDSFieldType.cc Registered new field types in the field‐type map
src/BDSFieldFactory.cc Wired new field classes into EM factory
src/BDSFieldEMCircularTM.cc Implemented Circular TM mode
src/BDSFieldEMAxialStandingApprox.cc Implemented axial standing-wave approximation
src/BDSFieldEMAxialFloquetApprox.cc Implemented axial Floquet approximation skeleton
src/BDSComponentFactory.cc Set user limits for RF steps; fixed half-wavelength bug
src/BDSCavityFieldType.cc Extended cavity‐field enum and mappings
src/BDSBeamlineIntegral.cc Added stub for axialstandingapprox energy integration
include/* Updated integrator, field, and cavity type headers
Comments suppressed due to low confidence (3)

src/BDSFieldType.cc:98

  • String mapping typo: "axialsfloquetapprox" should be "axialfloquetapprox" for consistency.
{

src/BDSFieldFactory.cc:73

  • Header filename typo: should be "BDSFieldEMAxialFloquetApprox.hh" to match the class name.
#include "BDSFieldEMAxlalFloquetApprox.hh"

include/BDSFieldEMAxlalFloquetApprox.hh:21

  • Header guard typo with extra 'B' prefix and misspelling; should be #define BDSFIELDEMAXIALFLOQUETAPPROX_H.
#define BBDSFIELDEMAXLALFLOQUETAPPROX_H

{"cavity_tphase", {"", 1.0}},
{"cavity_zphase", {"", 1.0}},
{"coilRadialThickness", {"m", CLHEP::m}},
{"cavity_zphase", {"", 1.0}},
Copy link

Copilot AI Jul 7, 2025

Choose a reason for hiding this comment

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

Duplicate entry for "cavity_zphase" in unitsFactorsMap will override the first; remove the redundant entry.

Suggested change
{"cavity_zphase", {"", 1.0}},

Copilot uses AI. Check for mistakes.
Comment on lines +90 to +100
"cavity_efield", /* TODO not needed */
"cavity_travelling", /* TODO not needed */
"cavity_radius", /* TODO not needed */
"cavity_length", /* TODO not needed */
"cavity_m", /* TODO not needed */
"cavity_n", /* TODO not needed */
"cavity_p", /* TODO not needed */
"cavity_tphase", /* TODO not needed */
"cavity_zphase", /* TODO not needed */
Copy link

Copilot AI Jul 7, 2025

Choose a reason for hiding this comment

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

[nitpick] Remove or resolve the "TODO not needed" comments on keys to reduce clutter and clarify which cavity parameters are actually required.

Suggested change
"cavity_efield", /* TODO not needed */
"cavity_travelling", /* TODO not needed */
"cavity_radius", /* TODO not needed */
"cavity_length", /* TODO not needed */
"cavity_m", /* TODO not needed */
"cavity_n", /* TODO not needed */
"cavity_p", /* TODO not needed */
"cavity_tphase", /* TODO not needed */
"cavity_zphase", /* TODO not needed */
// Removed unnecessary keys: cavity_efield, cavity_travelling, cavity_radius, cavity_length, cavity_m, cavity_n, cavity_p, cavity_tphase, cavity_zphase

Copilot uses AI. Check for mistakes.
BDSIntegratorType::g4classicalrk4, // rfpillbox
BDSIntegratorType::g4classicalrk4, // transversemagnetic
BDSIntegratorType::g4classicalrk4, // axialstandingapprox
BDSIntegratorType::g4classicalrk4, // axialsfloquetapprox
Copy link

Copilot AI Jul 7, 2025

Choose a reason for hiding this comment

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

Comment typo: "axialsfloquetapprox" should read "axialfloquetapprox" to match the field type.

Suggested change
BDSIntegratorType::g4classicalrk4, // axialsfloquetapprox
BDSIntegratorType::g4classicalrk4, // axialfloquetapprox

Copilot uses AI. Check for mistakes.
case BDSFieldType::transversemagnetic:
{return transversemagnetic; break;}
case BDSFieldType::axialstandingapprox:
{return axialstandingapprox; break;}
Copy link

Copilot AI Jul 7, 2025

Choose a reason for hiding this comment

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

The Integrator switch is missing a case for axialfloquetapprox; add case BDSFieldType::axialfloquetapprox: return axialfloquetapprox;.

Suggested change
{return axialstandingapprox; break;}
{return axialstandingapprox; break;}
case BDSFieldType::axialfloquetapprox:
{return axialfloquetapprox; break;}

Copilot uses AI. Check for mistakes.
G4double period = 1. / (element->frequency*CLHEP::hertz);
// choose the smallest length scale based on the length of the component of the distance
// travelled in one period - so improved for high frequency fields
G4double limit = 1;
Copy link

Copilot AI Jul 7, 2025

Choose a reason for hiding this comment

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

Variable limit is hardcoded to 1 instead of using period * stepFraction; update to limit = period * stepFraction for correct RF step sizing.

Suggested change
G4double limit = 1;
G4double limit = period * stepFraction;

Copilot uses AI. Check for mistakes.
}
case BDSCavityFieldType::axialstandingapprox:
{
//dEk = particleCharge *;
Copy link

Copilot AI Jul 7, 2025

Choose a reason for hiding this comment

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

Placeholder in axialstandingapprox case is incomplete; implement the energy change calculation or remove the stub.

Suggested change
//dEk = particleCharge *;
// Implement energy change calculation for axialstandingapprox cavity field type
G4double transitTimeFactor = BDSFieldEMRFCavity::TransitTimeFactor(frequency, phase, thisComponentArcLength, designParticle.Beta());
dEk = particleCharge * eField * thisComponentArcLength * transitTimeFactor * std::cos(phase);
break;

Copilot uses AI. Check for mistakes.
@stewartboogert stewartboogert force-pushed the acceleration branch 3 times, most recently from 19559ab to 741d88e Compare July 29, 2025 20:52
@stewartboogert stewartboogert force-pushed the acceleration branch 2 times, most recently from c8d0b7a to 35254a4 Compare October 4, 2025 12:17
@stewartboogert stewartboogert force-pushed the acceleration branch 9 times, most recently from d87429f to 8345dbf Compare October 17, 2025 20:51
stewartboogert and others added 3 commits October 31, 2025 16:10
Former-commit-id: 58452c9056ef0dbf2698a9a506452f963b0e2086
Former-commit-id: 8f9ffc9a203baf4d4f55f63a4d3ff1ab4f908c5d
Former-commit-id: 3293a94763a56a484404c83e666132a80168d692
stewartboogert and others added 29 commits October 31, 2025 16:10
Former-commit-id: 035fb3cb56eb695b34179095596a13f6521509ae
Former-commit-id: f44ebde262ed474b96ef776e247d743946274585
Former-commit-id: 87a533dd6af5e4c4c7cbb5b68f836d9f88c37547
Former-commit-id: c8f30b39ce5fa03f80aa6656a9c57012fde1fb97
Former-commit-id: 62eff660968aecc3c778d4a44b51bb8a0db4a53e
Former-commit-id: d963f91867acc1c3a0a44d3a7bbd0796d9853ef9
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.

3 participants