Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/MultiMode #295

Merged
merged 62 commits into from
Jun 26, 2024
Merged

Feature/MultiMode #295

merged 62 commits into from
Jun 26, 2024

Conversation

jkgaison65
Copy link
Contributor

Updates DHO model to accommodate independent configurations for different modes

Copy link
Contributor

@pslocum pslocum left a comment

Choose a reason for hiding this comment

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

These are exciting updates, thank you! I have added a few comments. Mainly I am wondering if we should replace the existing functions with your multimode upgrade, since it is more general. (e.g. replace fFilterComplex with fFilterComplexArray.) I can help with this too. Will follow up offline for more thoughts. Thanks again!

Comment on lines 226 to 227
if(bTE==1) term1 = fFieldCore->GetBesselNKPrimeZeros(l,m) / GetDimR();
else term1 = fFieldCore->GetBesselNKZeros(l,m) / GetDimR();
Copy link
Contributor

Choose a reason for hiding this comment

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

can we add the full {} syntax for the if () {} else {} ? (The shorthand notation here has been a gotcha for me in the past.)

Comment on lines 231 to 232
if(bTE==1)lambda_c = 2 * LMCConst::Pi() * GetDimR() / fFieldCore->GetBesselNKPrimeZeros(l,m);
else lambda_c = 2 * LMCConst::Pi() * GetDimR() / fFieldCore->GetBesselNKZeros(l,m);
Copy link
Contributor

Choose a reason for hiding this comment

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

can we add the full {} syntax for the if () {} else {} ?

int fTFNBins;
int fFIRNBins;
int fCropIndex;
std::vector < std::vector < std::vector < std::vector < int >>>> fFIRNBinsArray;
Copy link
Contributor

Choose a reason for hiding this comment

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

is fFIRNBins allowed to be different for each mode lmn? if not, I am wondering if we could use fFIRNBins to replace fFIRNBinsArray.

double fResolution;
std::vector < std::vector < std::vector < std::vector < double >>>> fResolutionArray;
Copy link
Contributor

Choose a reason for hiding this comment

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

is fResolution allowed to be different for each mode lmn? if not, I am wondering if we could use fResolution to replace fResolutionArray.

double fCharacteristicImpedance;
int fNSkips;
bool fHFSSFiletype;
ComplexFFT fComplexFFT;
bool fIsFIRCreated;
std::vector < std::vector < std::vector < std::vector < bool >>>> fIsFIRCreatedArray;
Copy link
Contributor

Choose a reason for hiding this comment

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

is fIsFIRCreated allowed to be different for each mode lmn? if not, I am wondering if we could use fIsFIRCreated to replace fFIRCreatedArray.

protected:

// Member variables
std::string fHFSSFilename;
std::vector<double> fFilter;
fftw_complex* fFilterComplex;
std::vector< std::vector< std::vector < std::vector< fftw_complex*>>>> fFilterComplexArray;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is where I am wondering if we could possibly just upgrade to your fFilterComplexArray, and delete references to the existing fFilterComplex.

@@ -97,7 +113,7 @@ namespace locust
virtual bool Configure( const scarab::param_node& aNode) override;
bool ReadHFSSFile() override;
bool ConvertAnalyticTFtoFIR(double initialFreq, std::vector<std::complex<double>> tfArray);
bool ConvertAnalyticGFtoFIR(std::vector<std::pair<double,std::pair<double,double> > > gfArray);
bool ConvertAnalyticGFtoFIR(int bTE, int l, int m, int n, std::vector<std::pair<double,std::pair<double,double> > > gfArray);
Copy link
Contributor

Choose a reason for hiding this comment

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

similar to the above, can we just upgrade to your new implementation, and remove the old implementation altogether?

Comment on lines 369 to 376
/*
double CylindricalCavity::TotalFieldNorm(std::vector<double> field)
{
double norm = 0;
auto it = field.begin();
while (it != field.end())
{
if (!isnan(*it)) norm += (*it)*(*it);
Copy link
Contributor

Choose a reason for hiding this comment

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

is TotalFieldNorm not needed any more?

virtual std::vector<double> GetNormalizedModeField(int l, int m, int n, std::vector<double> tKassParticleXP, bool includeOtherPols, bool teMode) {return {0.};};
double NormalizedEFieldMag(std::vector<double> field);
virtual std::vector<std::vector<std::vector<double>>> CalculateNormFactors(int nModes, bool bTE) {return {{{0.}}};};
virtual std::vector<double> GetTE_E(int l, int m, int n, double r, double theta, double z, bool includeOtherPols) {return {0.};};
virtual std::vector<double> GetTM_E(int l, int m, int n, double r, double theta, double z, bool includeOtherPols) {return {0.};};
virtual double CalculateDotProductFactor(int l, int m, int n, std::vector<double> tKassParticleXP, std::vector<double> aTE_E_normalized, double tThisEventNSamples) {return {0.};};
virtual double CalculateDotProductFactor(int bTE, int l, int m, int n, std::vector<double> tKassParticleXP, std::vector<double> aTE_E_normalized, double tThisEventNSamples) {return {0.};};
Copy link
Contributor

Choose a reason for hiding this comment

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

is the bTE required for the dot product calculation, given that the E-field and e- velocity are inputs? lmk your thoughts.

@pslocum pslocum changed the base branch from master to develop November 13, 2023 17:57
@jkgaison65
Copy link
Contributor Author

Expansion of MultiMode features to a broader range of parameters, such as DHO Threshold and frequency (usually not needed, but may be needed if we ever look at higher harmonics). Also generalized configurable parameters beyond just TE011 and TM111 modes

@pslocum pslocum marked this pull request as ready for review June 17, 2024 13:56
@pslocum pslocum merged commit 5a187e0 into develop Jun 26, 2024
2 checks passed
@pslocum pslocum deleted the feature/MultiMode branch June 26, 2024 16:08
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