-
Notifications
You must be signed in to change notification settings - Fork 3
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/sensitivity curve link rf background and detection efficiency to snr #73
base: feature/sensitivity_curve
Are you sure you want to change the base?
Conversation
…ign_detection_efficiency_from_threshold() in the init so the new features are added
It looks to me like use_threshold is a parameter in the config file that isn't used in the code. Am I missing a usage of that parameter? Or should it be removed from the config file? |
I made the configuration use_threshold active in the code. It makes it more flexible when running the code. If use_threshold = False, the processor can bypass using the calculated SNR. |
@@ -154,6 +154,12 @@ def __init__(self, config_path): | |||
self.EffectiveVolume() | |||
self.PitchDependentTrappingEfficiency() | |||
self.CavityPower() | |||
try: | |||
self.Threshold = NameSpace({opt: eval(self.cfg.get('Threshold', opt)) for opt in self.cfg.options('Threshold')}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know why this line is needed for the Threshold and Efficiency sections of the config file, but not for all the other sections of the config file? Did you try running the code without this line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. For configure files without a [Threshold] section, it needs the try:
to run the processor. I was running the script with the configuration for Phase II without using try:
and saw an error relating to [Threshold] not found. The other scripts all have an [Efficiency] section. So reading the [Efficiency] parameters doesn't require a try:
section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Yu-Hao – my question wasn't clear enough. I was actually wondering why you need line 158, not why you need a try:
statement.
For both the Efficiency and the Threshold blocks, there is a line in the code like:
self.Efficiency = NameSpace({opt: eval(self.cfg.get('Efficiency', opt)) for opt in self.cfg.options('Efficiency')})
However, for the Experiment block and the FrequencyExtraction block, there is no such line. The code just reads variables from the config file like this:
self.CRLB_constant = self.FrequencyExtraction.crlb_constant
without first needing a line with NameSpace
in it.
Do you know why this is?
self.assign_background_rate_from_threshold() | ||
self.assign_detection_efficiency_from_threshold() | ||
except: | ||
pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest adding logger.info outputs here.
If Threshold.use_threshold == False, the output should say that the background rate and detection efficiency from the config file is being used.
If Threshold.use_threshold == False, the output should say that any background rate and detection efficiency inputs in the config file are being overridden.
Per my other comment, I do think you should change the variables to "detection_threshold" and "use_detection_threshold," to be more descriptive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The above should say if self.Threshold.use_threshold
, not if Threshold.use_threshold
. I've fixed this in my local repository and will push the change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@casesyh , flagging the first comment above. I only addressed the second comment above, not the first one.
…rom the wall if it's larger than the inputted distance
…round related functions to improve readdability
…y() methods to reference Rene's slides https://3.basecamp.com/3700981/buckets/3107037/documents/8013439062
…tion -> mean_track_duration and tau -> track_duration. Tested that the processor runs.
…except section in __init__(). Tested that the script runs.
…ment_max_BNL_diam.cfg
…ion_efficiency_to_snr' of https://github.com/project8/mermithid into feature/sensitivity_curve_link_rf_background_and_detection_efficiency_to_snr
There's one issue with your latest changes: these two lines |
…h density in the density optimization
…erating density in sens vs density plots, added option to not optimize density in sens vs exposure-type plots, swapped the left and right y-axes in sens vs exposure-type plots, added single Phase IV cavity to example plots, accounted properly for the case where the threshold isn't specified (fixed efficiency) or the trap length isn't specified, improved plot formatting
I made the following additional changes:
|
The added feature is to consider the effect of the SNR on the background rate and the detection efficiency based on René's work: https://3.basecamp.com/3700981/buckets/3107037/documents/8013439062. https://3.basecamp.com/3700981/buckets/3107037/documents/7572536796#__recording_7616690029. The configure file is in https://github.com/project8/termite/blob/background_rate_and_detection_efficiency_as_functions_of_SNR/sensitivity_config_files/Config_LFA_Experiment_max_BNL_diam.cfg. The processor is running for now. I'm sending a pull request.