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

Combining multi gas scattering with simulated ins resolution in fake data generator #53

Open
wants to merge 105 commits into
base: develop
Choose a base branch
from

Conversation

taliaweiss
Copy link
Collaborator

@taliaweiss taliaweiss commented Feb 8, 2021

The combining_... branch allows for a simulated instrumental resolution distribution to be used when fitting Kr data and generating fake T2 data. It also includes various updates to the complex lineshape model, such as incorporating the reconstruction efficiency curve as a function of scattering peak order.

casesyh and others added 30 commits June 25, 2020 23:03
Copy link
Contributor

@cclaessens cclaessens left a comment

Choose a reason for hiding this comment

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

this really looks like a great amount of work! thank you all for completing this branch and implementing the many options the multi gas complex lineshape processor now has.

I have a few general remarks/requests/questions:

  1. Do we still need the KrComplexLineshape processor now we have the MultiGasLineshapeProcessor?
  2. Please add more descriptions to the lineshape processor functions and give the functions better names than make_spectrum_1 and fit_data_2. For some options you already did that and it makes it much more understandable what the functions do.
  3. The MultiGasComplexLineshape processor is too long (>3000 lines). Can you move the small support functions to the utility file.
  4. There are features in the MultiGasComplexLineshape processor that we will probably not use for the final analysis and I think they should not be merged to develop: for example the smeared triangle resolution and the artificial scaling of resolution widths.

raw_data = [float(i) for i in line.split("\t")]
energyOsc[0].append(raw_data[0])
energyOsc[1].append(raw_data[1])
except:
Copy link
Contributor

Choose a reason for hiding this comment

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

why is there no warning or error when the try fails?
should energyOsc not always be read in from the file?

@@ -109,11 +119,10 @@ def energy_guess_to_frequency(energy_guess, energy_guess_err, B_field_guess):
return frequency , frequency_err

# Given a frequency and error, converts those to B field values assuming the line is the 17.8 keV line
Copy link
Contributor

Choose a reason for hiding this comment

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

update comment. error is no longer a parameter here.

mermithid/misc/FakeTritiumDataFunctions.py Outdated Show resolved Hide resolved
mermithid/misc/FakeTritiumDataFunctions.py Outdated Show resolved Hide resolved
mermithid/misc/FakeTritiumDataFunctions.py Outdated Show resolved Hide resolved
return normalized_convolved_spectrum

#might be untested
def convolve_smeared_triangle(self, func_to_convolve, center, scale1, scale2, exponent, sigma):
Copy link
Contributor

Choose a reason for hiding this comment

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

if we remove smeared triangle this should also be removed.

}
return dictionary_of_fit_results

# same as make_spectrum except that scatter_proportion is fixed
Copy link
Contributor

Choose a reason for hiding this comment

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

I am glad there is description here in comments. I still think the make_spectrum functions should have slightly more descriptive names than _N

current_full_spectrum += coefficient*current_working_spectrum*prob_parameter**M
return current_full_spectrum

def spectrum_func_1(self, bins_Hz, *p0):
Copy link
Contributor

Choose a reason for hiding this comment

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

also more descriptive name and comment for spectrum_func_N

f[nonzero_idx] += amplitude*f_intermediate[nonzero_idx]/np.sum(f_intermediate[nonzero_idx])
return f

def fit_data_1(self, freq_bins, data_hist_freq):
Copy link
Contributor

Choose a reason for hiding this comment

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

same here. what's the difference to fit_data?

}
return dictionary_of_fit_results

# using simulated resolution, with multi gas scattering, reconstruction eff, without detection eff, has been used in fake data generator. However, without using detection eff is the right option for tritium fake data generation.
Copy link
Contributor

Choose a reason for hiding this comment

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

split comments over multiple lines for readability. in general, function descriptions should come after def function_name(). and they should go in between ''' '''.

@taliaweiss
Copy link
Collaborator Author

taliaweiss commented Feb 12, 2021

Thanks very much for reviewing the branch, Christine. To your questions:

  1. I think we should remove the KrComplexLineshape processor. Yu-Hao should confirm (@casesyh).

2-3. I believe these are items for Yu-Hao to follow up on.

  1. I agree that we should remove/avoid merging features of the MultiGasComplexLineshape processor that we will not be using, like the smeared triangle resolution. I might suggest retaining the artificial scaling of widths as an option, because I could see that being useful for our systematics analysis.

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.

4 participants