feature/cavity_noise_simulation (additive noise implementation) #210
Conversation
… a working seed for random noise generation
…pectrum issue in PS)
…nal noise handling
…d corrected cavity noise model power from the PSD
ehtkarim
force-pushed
the
feature/cavity_noise_simulation
branch
from
94b00d2 to
9be0933
Compare
…enerator' processor. Updated 'CMakeLists.txt'
|
ehtkarim
changed the title
There was a problem hiding this comment.
Please rename this to KatydidPSNoiseConfig.yaml to stick with CamelCase and have the "noise" modifier before "Config"
There was a problem hiding this comment.
Please rename this to KatydidPSNoiseCavityConfig.yaml to stick with CamelCase and have the "NoiseCavity" modifier before "Config"
| virtual bool ConfigureDerivedGenerator(const scarab::param_node* node); | ||
|
|
||
| protected: | ||
| struct ModelPars |
There was a problem hiding this comment.
Why are the model parameters contained within a struct? I don't see any particular design reason for it. They should just be member variables of the processor.
I suggest making them all member variables in the class using the MEMVAR macros. Try to stick with the CamelCase naming convention.
| const double g = fPars.Q0 / fPars.Q_L; | ||
| const double lor = 1.0 / ( 1.0 + std::pow( 2.0 * fPars.Q_L * (f - fPars.f0) / fPars.f0, 2.0 ) ); // Lorentzian | ||
|
|
||
| const double kB = 1.380649e-23; |
There was a problem hiding this comment.
These constants should be added to KTMath (in the Utility directory). Please add them there.
Also, while you're editing those files, can you please make the functions that return numeric constants constexpr? Originally the minimum version of C++ I used didn't have that when I first started Katydid, but we require C++17, so we an use it.
|
|
||
| double KTCavityNoiseGenerator::NoisePSD(double f) const | ||
| { | ||
| const double g = fPars.Q0 / fPars.Q_L; |
There was a problem hiding this comment.
I suggest you add g as a private member variable (no setter). And then you calculate it in the setters for Q0 and Q_L. That way you're not calculating it every time NoisePSD is calculated.
|
|
||
| const double P_cav = kB*Tcav * (4.*g/std::pow(1.+g,2)) * lor; | ||
| const double P_loss = kB*( fPars.A*fPars.T_line_start + (1.-fPars.A)*fPars.T_line_end ); | ||
| const double P_isol = kB*Tisol * (1. - (4.*g/std::pow(1.+g,2))*lor); |
There was a problem hiding this comment.
pow() is quite slow. For a simple thing like this one I suggest you just use (1.+g)*(1.+g).
| const double hbar= 1.054571817e-34; | ||
| const double omega = 2.0 * M_PI * f; | ||
|
|
||
| auto eta = [](double x){ return x/(std::exp(x)-1.0) + 0.5; }; // Bose-Einstein factor |
There was a problem hiding this comment.
I suggest implementing this as a member function. That way you can have unit tests for it, and you won't have memory being allocated for the lambda at runtime.
| - egg | ||
|
|
||
| egg: | ||
| filename: "locust_mc_Seed600_Angle90.000000000_Pos0.0040000_Energy18563.251000000.egg" |
There was a problem hiding this comment.
Since this is an example, please give a generic filename.
| - egg | ||
|
|
||
| egg: | ||
| filename: "locust_mc_Seed600_Angle90.000000000_Pos0.0040000_Energy18563.251000000.egg" |
There was a problem hiding this comment.
Please give a generic filename
|
|
||
| protected: | ||
| // Simple natural cubic spline for doubles | ||
| class KCubicSpline |
There was a problem hiding this comment.
Instead of re-implementing a spline here, I suggest using Utility/KTSpline
There was a problem hiding this comment.
Actually, you could take your implementation and replace the use of ROOT's TSpline3. I've always been bothered by requiring ROOT for the use of KTSpline, but never enough to fix it.
The processor "gaussian-noise-generator" (from
KTGaussianNoiseGenerator.cc) now correctly adds White Gaussian Noise to the signal. I've added an example config file (namedKatydidPSConfig_noise.yaml) demonstrating the processor usage. One can also use a random number seed using the "seed" parameter in the config file