Fix tick bug when expanding electric potential to full 2pi #310
Conversation
|
The field calculations seems to work but I can't plot the electric potential and electric field in r-phi: plot(sim.electric_potential, z= 0) # -> "angles not sorted in ascending order."Could you please have a look? |
|
Yes, the ticks should be ordered, and ideally, the first tick of a g = Grid(sim)
g.φ.ticks[1] == g.φ.interval.left |
SebastianRuffert
force-pushed
the
CylGridExpansion
branch
from
82f105b to
31ee1d0
Compare
| P = sortperm(new_ticks) | ||
| new_int::Interval{:closed, :open, AT} = Interval{:closed, :open, AT}(new_ticks[P][1], new_ticks[P][end]) | ||
| new_axφ = DiscreteAxis{AT, :periodic, :periodic}( new_int, new_ticks[P] ) |
There was a problem hiding this comment.
I do not think that this is what we want...
The endpoints of the Interval should not necessarily be the first and last ticks of new_ticks[P], especially when the Interval is :open on the right.
In this function (get_2π_potential), we want to end up with a 2π potential,and in my personal opinion the new_int should also be a 2π-Interval, i.e. where the :closed-:open-Interval ranges from [0,2π[ (or [x,x+2π[ but then again there might be values outside of the [0,2π[ range...)
| - name: "p contact" | ||
| id: 1 | ||
| material: "HPGe" | ||
| potential: 4000. |
There was a problem hiding this comment.
Minor comment: p+ contacts usually have a negative potential applied
| detectors: | ||
| - semiconductor: | ||
| material: "HPGe" | ||
| bulk_type: "p" |
There was a problem hiding this comment.
bulk_type, at least to my knowledge, should be ignored (?)
SebastianRuffert
force-pushed
the
CylGridExpansion
branch
from
0a401a6 to
cc6afb8
Compare
| @@ -86,7 +86,7 @@ function get_electric_field_from_potential(epot::ElectricPotential{T, 3, Cylindr | |||
| Δp_φ_1 = p[ir ,iφ+1, iz]-p[ir ,iφ, iz] | |||
| Δp_φ_2 = p[ir ,iφ, iz]-p[ir ,end, iz] | |||
| d_φ_1 = (axφ[iφ+1]-axφ[iφ]) * axr[ir]# to get the proper value in length units | |||
| d_φ_2 = (cyclic - axφ[end]) * axr[ir] | |||
| d_φ_2 = (cyclic + axφ[iφ] - axφ[end]) * axr[ir] | |||
There was a problem hiding this comment.
Do we really need this?
Right now we always blow up the electric potential to full 2π before calling this function.
So wouldn't axφ[iφ] (iφ = 1 here) be always 0?
|
Why was this closed? |
|
@fhagemann What is the status of this? |
|
I did not yet look into this in detail, might get to this next week. What I see is that we added new config files without corresponding test. It might make sense to add tests for a phi interval not starting at 0 to make sure that everything runs smoothly before merging this into main. |
|
If I have a look at the new config file introduced in this PR ( using SolidStateDetectors, Unitful, Plots
sim = Simulation(SSD_examples[:ConeSym])
calculate_electric_potential!(sim)
plot(sim.point_types, z = 0.009)
Not sure exactly what the problem is here (maybe missing important ticks?) but this is something to look into before merging. |
This happens to be due to rounding errors |
| new_ticks_new_zero[2:end] = new_ticks[P] | ||
| new_pot_new_zero = Array{eltype(sp.data), 3}(undef, size(sp, 1), l * n + 1, size(sp, 3)) | ||
| new_pot_new_zero[:,1,:] = (eltype(sp.data) <: AbstractFloat) ? sp[:,1,:] + | ||
| (sp[:,end,:] - sp[:,1,:]) * (T(0) - new_ticks_new_zero[2]) / (T(2π) - T(new_ticks_new_zero[end]) * n + T(new_ticks_new_zero[2])) : |
There was a problem hiding this comment.
I feel like this formula is wrong and it should contain something like Δφ * n rather than T(new_ticks_new_zero[end]) * n.. I will do the calculation and see if this needs to be adjusted.
There was a problem hiding this comment.
The permutation of the new potential based on the permutation of the new ticks is missing here, I will push some changes to fix this..
This should solve issue #253