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

Allow for contact_id to be any Int #290

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

SebastianRuffert
Copy link
Contributor

@SebastianRuffert SebastianRuffert commented May 12, 2022

This should solve issue #288

@fhagemann
Copy link
Collaborator

Thank you.
I saw that also changes in the I/O. Will this change allow to read in saved simulations (so no changes for the user)?

@fhagemann
Copy link
Collaborator

And what happens if two contacts have the same ID in the config file?

@fhagemann
Copy link
Collaborator

Can you add tests where the contact IDs are not in order?
And maybe also a case in which two contacts have the same ID and running the code fails.

@SebastianRuffert
Copy link
Contributor Author

Thank you. I saw that also changes in the I/O. Will this change allow to read in saved simulations (so no changes for the user)?

Partly yes. Before, the different weighting potentials were initialized inside a list, while now that is being done in a dictionary, so now changes for the user in the rea-in of a config file. At least not ones, that I can think of now.
What changes is the final struct. Meaning, if the user wants to access sim.weighting potentials a dict will be returned instead of a list.

@SebastianRuffert
Copy link
Contributor Author

And what happens if two contacts have the same ID in the config file?

No error is thrown right now, when reading in a config file with two contacts of id: 1 and using simulate()

julia> sim = Simulation{Float32}("Software/SolidStateDetectors.jl/examples/example_config_files/contact_id_same_id.yaml")
julia> simulate!(sim)
Simulation{Float32, Cartesian} - Coordinate system: Cartesian
  Environment Material: High Purity Germanium
  Detector: Contact ID test file
  Electric potential: (58, 34, 34)
  Charge density: (58, 34, 34)
  Impurity scale: (58, 34, 34)
  Fix Charge density: (58, 34, 34)
  Dielectric distribution: (59, 35, 35)
  Point types: (58, 34, 34)
  Electric field: (58, 34, 34)
  Weighting potentials: 
    Contact 1: (14, 12, 12)
    Contact 1: (14, 12, 12)

But when accessing the weighting potentials directly, only one is shown

julia> sim.weighting_potentials
OrderedCollections.OrderedDict{Int64, Union{Missing, WeightingPotential}} with 1 entry:
  1 => [0.999674 0.999675 … 0.999684 0.999682; 0.999672 0.999676 … 0.999684 0.999681; … ; 0.999682 0.999686 … 0.999696 0.999692; 0.999684 0.999687 ……

How do we want to handle that case? We can throw an error if there is ambiguity in the contact ids. Another option is changing the ambigous contact id internally, maybe by adding one to the id or multiplying by 10.

@SebastianRuffert SebastianRuffert force-pushed the ContactID branch 2 times, most recently from 00c6657 to 5b60b2c Compare May 17, 2022 12:36
@lmh91
Copy link
Collaborator

lmh91 commented May 24, 2022

I get an error in

sim = Simulation(SSD_examples[:ContactIDTest])
simulate!(sim)
calculate_capacitance_matrix(sim)

@lmh91
Copy link
Collaborator

lmh91 commented May 24, 2022

Also,

sim = Simulation(SSD_examples[:ContactIDTest])
simulate!(sim)
calculate_weighting_potential!(sim, 4)

does actually calculate some field and stores it into the dict.
This should throw an error.

@lmh91
Copy link
Collaborator

lmh91 commented May 24, 2022

And what happens if two contacts have the same ID in the config file?

How do we want to handle that case? We can throw an error if there is ambiguity in the contact ids. Another option is changing the ambigous contact id internally, maybe by adding one to the id or multiplying by 10.

No, we should throw an error in the construction of the SolidStateDetector.

@SebastianRuffert
Copy link
Contributor Author

sim = Simulation(SSD_examples[:ContactIDTest])
simulate!(sim)
calculate_weighting_potential!(sim, 4)

All calls of this function already use contact.id

@lmh91
Copy link
Collaborator

lmh91 commented Jun 7, 2022

calculate_weighting_potential!(sim, 4)

But there is no contact with id == 4. That's the issue.

Comment on lines 553 to 558
contact_ids = [contact.id for contact in sim.detector.contacts]
if contact_id in contact_ids
sim.weighting_potentials[contact_id] = WeightingPotential(ElectricPotentialArray(pcs), grid)
else
@error "No such contact"
end
Copy link
Collaborator

Choose a reason for hiding this comment

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

To avoid any unnecessary computations, please move the check if a contact with the
respective id exists into the function calculate_weighting_potential! at the very beginning.

Please also add a bit more information for the user:

!(contact_id in sim.detector.contacts) && @error "No contact with `ID = $(contact_id)`. Possible contact IDs are $(...)"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

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.

3 participants