Conversation
marcomuzio
left a comment
marcomuzio
left a comment
There was a problem hiding this comment.
Thanks @AlanSalcedo ! Just a few small things.
IceModel.cc
Outdated
|
|
||
| setUpIceModel(model); | ||
|
|
||
| const char* araSimDir = getenv("ARA_SIM_DIR"); |
There was a problem hiding this comment.
I think this is already defined in the header as ara_sim_dir
IceModel.cc
Outdated
| } | ||
| else { | ||
| std::string attenFile = | ||
| std::string(araSimDir) + "/data/iceattenuation_systematics.txt"; |
There was a problem hiding this comment.
Does this file exist? Or is this just a placeholder for something that needs to be added?
| std::string(araSimDir) + "/data/iceattenuation_systematics.txt"; | ||
|
|
||
| if (!LoadIceAttenPercentTable(attenFile)) { | ||
| std::cerr << "Warning: failed to load ice attenuation systematics file: " |
There was a problem hiding this comment.
Should this throw an error? Or is the idea we always try to load it but maybe we don't always use it?
IceModel.cc
Outdated
| } | ||
|
|
||
| double IceModel::GetIceAttenSystematicsFactor(double depth, Settings *settings1) const { | ||
| if (!settings1) { |
There was a problem hiding this comment.
Is this to make it like an optional argument?
There was a problem hiding this comment.
This is unnecessary, let me drop it
| return 1.0; | ||
| } | ||
|
|
||
| if (!iceAttenPctTableLoaded || iceAttenPctDepth.empty()) { |
There was a problem hiding this comment.
Okay maybe now this should be a real crash? I think if you've gotten this far into the function the user is intending to calculate the systematics?
|
|
||
| // read depth in positive value and return attenuation length (m) at the depth | ||
| double IceModel::GetARAIceAttenuLength(double depth) { | ||
| double IceModel::GetARAIceAttenuLength(double depth, Settings *settings1) { |
There was a problem hiding this comment.
Do we also want to add the systematics support to EffectiveAttenuationLength?
There was a problem hiding this comment.
Fair enough. Only to the version that already uses has settings in the parameters, which is the only one used in report.cc.
| } | ||
|
|
||
|
|
||
| ApplyNofzSystematics(ns, nd, nc, settings1); |
There was a problem hiding this comment.
Doesn't this mean the ice model parameters are incremented in every call to this function? Are they always reset in the function too?
There was a problem hiding this comment.
No, ns, nd, nc are reset in every call, so the increments don't accumulate. That's how it was done already
RaySolver.cc
Outdated
|
|
||
|
|
||
| void RaySolver::ApplyNofzSystematics(double& ns, double& nd, double& nc, Settings* settings1) { | ||
| if (!settings1) return; |
There was a problem hiding this comment.
What's the use case here?
Settings.h
Outdated
|
|
||
| int SYSTEMATICS_IceAttenuation; // 0=central, 1=up, 2=low | ||
|
|
||
| int SYSTEMATICS_Askaryan; // 0=central (default), 1=increase pecentage, 2=decrease pecentage |
There was a problem hiding this comment.
also do we need this parameter and the one for the size of the change? they seem a little redundant.
There was a problem hiding this comment.
It just feels more natural to say 12% and then: add or subtract via this other flag.
There was a problem hiding this comment.
Instead of +12% and -12%?
There was a problem hiding this comment.
Okay, I settled for allowing + or - instead. So we avoid redundancy.
signal.cc
Outdated
| } | ||
|
|
||
| double Signal::GetAskaryanSystematicsFactor(Settings *settings1) { | ||
| if (!settings1) return 1.0; |
2 participants
This PR adds the ability to modify the askaryan signal amplitude, attenuation lengths (given a file), and n(z) parameters, so we can evaluate systematics on simulations.
All veff tests passed: