-
Notifications
You must be signed in to change notification settings - Fork 50
Refactor ThermalDiffusionLithiumDensity to remove hardcoded parameters #578
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
Refactor ThermalDiffusionLithiumDensity to remove hardcoded parameters #578
Conversation
examples/example_config_files/ThermalDiffusionLithiumDensity/Thermal_Diffusion_Config.yml
Outdated
Show resolved
Hide resolved
src/ImpurityDensities/ThermalDiffusionLithiumDensityParameters.jl
Outdated
Show resolved
Hide resolved
src/ImpurityDensities/ThermalDiffusionLithiumDensityParameters.jl
Outdated
Show resolved
Hide resolved
src/ImpurityDensities/ThermalDiffusionLithiumDensityParameters.jl
Outdated
Show resolved
Hide resolved
src/ImpurityDensities/ThermalDiffusionLithiumDensityParameters.jl
Outdated
Show resolved
Hide resolved
examples/example_config_files/ThermalDiffusionLithiumDensity/Thermal_Diffusion_Config.yml
Outdated
Show resolved
Hide resolved
|
I tried to understand where the error comes from, but it does not seem to be related to the changes I made. |
src/ImpurityDensities/ThermalDiffusionLithiumDensityParameters.jl
Outdated
Show resolved
Hide resolved
src/ImpurityDensities/ThermalDiffusionLithiumDensityParameters.jl
Outdated
Show resolved
Hide resolved
src/ImpurityDensities/ThermalDiffusionLithiumDensityParameters.jl
Outdated
Show resolved
Hide resolved
src/ImpurityDensities/ThermalDiffusionLithiumDensityParameters.jl
Outdated
Show resolved
Hide resolved
src/ImpurityDensities/ThermalDiffusionLithiumDensityParameters.jl
Outdated
Show resolved
Hide resolved
src/ImpurityDensities/ThermalDiffusionLithiumDensityParameters.jl
Outdated
Show resolved
Hide resolved
src/ImpurityDensities/ThermalDiffusionLithiumDensityParameters.jl
Outdated
Show resolved
Hide resolved
src/ImpurityDensities/ThermalDiffusionLithiumDensityParameters.jl
Outdated
Show resolved
Hide resolved
examples/example_config_files/ThermalDiffusionLithiumDensity/Thermal_Diffusion_Config.yaml
Show resolved
Hide resolved
src/ImpurityDensities/ThermalDiffusionLithiumDensityParameters.jl
Outdated
Show resolved
Hide resolved
src/ImpurityDensities/ThermalDiffusionLithiumDensityParameters.jl
Outdated
Show resolved
Hide resolved
6e3dcf9 to
5bf0306
Compare
5bf0306 to
7f69bc8
Compare
The error in the documentation build came from adding the unregistered package |
|
Ok, the |
The publication quotes values for calculating cm^-3, SSD expects m^-3
|
@fhagemann @SimonePellegrini To be clear, why I wrote 27.27 instead of 21.27 as the ref?: Because the default length unit is "m" in SSD.jl, but people prefer to use "cm" and "cm^3" when talking about impurity density. |
|
Yes, I figured that out after looking at the numbers :) |
|
Hi, could you undo pushing the example JSON file for the inverted coaxial detectors? :) |
I think I've done it :) |
fhagemann
left a comment
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.
There are some minor things to clean up, but I can take care of that!
src/ImpurityDensities/ThermalDiffusionLithiumDensityParameters.jl
Outdated
Show resolved
Hide resolved
fhagemann
left a comment
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 moved all the structs and functions for diffusivity and saturation to the new file ThermalDiffusionLithiumDensityParameters.jl, added doc strings and removed obsolete code.
I also removed the suffx _in_germanium from the methods calculating the diffusivity, because the references also allow to do the same thing in silicon (no need to restrict the naming here).
We could add tests to trigger all the ConfigErrors (what happens if the list is empty, if it's missing parameters, ...), but other than that, this should be good to go.
The publications used for germanium also seem to provide values for silicon. I do not see why this model should not be also applicable to silicon.
c111a44 to
ab8ec08
Compare
fhagemann
left a comment
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 think this should be it!
@zpdg If you're ok with renaming llthium_diffusivity_in_germanium to just lithium_diffusivity, this should be good ton be merged!
And thanks @SimonePellegrini for getting this started!
It was a pleasure helping you! I learned a lot of interesting things. I'm looking forward to getting better and being more helpful to the Julia ecosystem :) |
This restriction is unnecessary. I am okay with it. |
This PR addresses #567