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

ToDo | General #37

Open
4 of 11 tasks
sagitta42 opened this issue Jul 21, 2023 · 8 comments
Open
4 of 11 tasks

ToDo | General #37

sagitta42 opened this issue Jul 21, 2023 · 8 comments

Comments

@sagitta42
Copy link
Contributor

sagitta42 commented Jul 21, 2023

ToDo items that span several stages from detector to elec response simulation

  • try to process raw file with pygama -> actually first check if the format is still the same, this is format from 2 years ago
  • Currently have overwrite as input for detector simulation (overwrite SSD hdf5 or fieldgen WP and EP files) -> implement also for simulate_pulses and simulate_waveforms, maybe call "overwrite_det_sim" or something like that to not be confused with caching waveforms themselves
  • Add more tests
  • Add Tutorial notebook to repo
  • docstrings
  • Currently caching detector simulation only if cached name provided -> should do the same for pss and raw? stp? Or user can cache their table by themselves? -> currently simulate_waveforms can read cached pss file, but pss does not start from stp, only from pet -> think of caching in general, how to organize
  • Test on non-ICPC
  • Want @info to not say which line/module it prints from to avoid log clutter
  • Optional detector metadata in json config (currently only separate input) to have one config that contains all info. Convenient to save and document exactly with what input something was simulated. More convenient to have detector separate from env, sim and setup settings for looping purposes. Therefore should be optional input syntax.
  • If cached name given no need for other detector simulation related settings! Right now first announces the settings, then just says cached sim is being read, so all of those settings are irrelevant. Also impossible to make sure what settings were used reading from cached sim - cached name chosen by user, not required to mention settings there. Hypothetically probably settings are recoverable (e.g. can see impurity gradient, voltage, but maybe not DL)
  • {T} in Preamp etc -> @oschulz @fhagemann we should call and you explain to me what this T stuff is, why it's good, and some rule of thumb on what to do there
@sagitta42
Copy link
Contributor Author

sagitta42 commented Nov 17, 2023

@oschulz @fhagemann a small question: advice on how to name an optional bool variable in simulate_pulses() and simulate_waveforms() that corresponds to overwrite in simulate_fields() i.e. determines whether to read cached simulation if present with given cached name or overwrite it. I'm thinking simply overwrite could be ambiguous when talking about waveform simulation (even though the only thing written is always only detector simulation). So maybe something like overwrite_fields?

Then it would look something like this:

pss_table, pss_truth = LegendGeSim.simulate_pulses(detector_metadata_filename, pet_input_file,
    environment_settings, simulation_settings, noise_model; n_waveforms=100, overwrite_fields=true)

@oschulz
Copy link
Contributor

oschulz commented Nov 17, 2023

Which of the arguments of simulate_pulses contains data that would be overwritten?

@sagitta42
Copy link
Contributor Author

sagitta42 commented Nov 17, 2023

Nothing "contains" data that should be overwritten. The argument overwrite should be passed to stp_to_pss() in simulate_pulses() where it would then be passed to simulate_detector(). Currently in stp_to_pss() the function simulate_detector() is called with a hard-coded overwrite = false.

sim = simulate_detector(det_meta, env, simulator; overwrite = false)

@fhagemann
Copy link
Contributor

How about, in general, using recalculate (or recalculate_fields) rather than overwrite (or overwrite_fields)?

@sagitta42
Copy link
Contributor Author

I like it! Maybe some how include the word "cached" in the variable, e.g. overwrite_cached to refer to the "cached name" in the simulation settings. That could be intuitive. The most descriptive and accurate name would be recalculate_cached_fields, but do we want an optional variable name be that long...

@oschulz
Copy link
Contributor

oschulz commented Nov 18, 2023

Makes sense.

@fhagemann
Copy link
Contributor

Question is: what exactly does the keyword do?

I feel like there might be three options which might be wanted by the user:

  • calculate the fields but discard them at the end of the simulation without saving them
  • calculate the fields and save them to the cache (file)
  • load the fields from the cache (file)

We might need two keywords for this, something like save_fields and load_cached_fields?
Or is any of the three scenarios never the case? Then, we might be able to work with one keyword argument and can be fixed according to what we actually select.

@sagitta42
Copy link
Contributor Author

This keyword controls whether to read cached simulation from file based on given cached name or overwrite it.

Option 1 is achieved by not providing a cached name in the simulation, then it doesn't matter what overwrite is.

Option 2 calculate the fields and cache them to a file - currently done if file does not exist yet; if it exists, the fields will be read unless overwrite is true (= option 3)

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

No branches or pull requests

3 participants