Reference model corrections (new db simulation model) #1331
Conversation
…-model-corrections
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
orelgueta
left a comment
orelgueta
left a comment
There was a problem hiding this comment.
A couple of comments and the following question:
Should I check the values you changed one by one in detail or should I trust they are OK? From memory, they look fine, but that's not enough if they need an actual review.
| type: | ||
| - MSTN | ||
| - MSTS | ||
| - SSTS |
There was a problem hiding this comment.
Why did you remove only SSTs if it is relevant only for FlashCams?
There was a problem hiding this comment.
Same, see above.
| type: | ||
| - MSTN | ||
| - MSTS | ||
| - SSTS |
There was a problem hiding this comment.
Why did you remove only SSTs if it is relevant only for FlashCams?
There was a problem hiding this comment.
Same, see above.
| type: | ||
| - MSTN | ||
| - MSTS | ||
| - SSTS |
There was a problem hiding this comment.
Why did you remove only SSTs if it is relevant only for FlashCams?
There was a problem hiding this comment.
Same, see above.
| type: | ||
| - MSTN | ||
| - MSTS | ||
| - SSTS |
There was a problem hiding this comment.
Why did you remove only SSTs if it is relevant only for FlashCams?
There was a problem hiding this comment.
Same, see above.
| type: | ||
| - MSTN | ||
| - MSTS | ||
| - SSTS |
There was a problem hiding this comment.
Why did you remove only SSTs if it is relevant only for FlashCams?
There was a problem hiding this comment.
Same, see above.
| type: | ||
| - MSTN | ||
| - MSTS | ||
| - SSTS |
There was a problem hiding this comment.
Why did you remove only SSTs if it is relevant only for FlashCams?
There was a problem hiding this comment.
Same, see above.
| data: | ||
| - type: double | ||
| unit: ns | ||
| default: 100. |
There was a problem hiding this comment.
The references below have a 1000, but the default here is 100. I guess that's the default in sim_telarray, but should we follow that or what is most common for us?
There was a problem hiding this comment.
Agree. I've set it to the sim_telarray default. Changed to 1000.
|
@orelgueta - thanks for the comments. Changed the dsum options to MSTs only and the default for array_window. Note:
|
This comment has been minimized.
This comment has been minimized.
OK!
Why do you say that? I thought we saw in this exercise that these lists are used to decide if to plot a parameter or not.
I don't agree, I do think we need both. The default values in the schema file should be reasonable ones while the dummy telescope should include mandatory parameters with clearly wrong parameters so that if there is an issue with one of the telescopes defined, the program crash instead of defaulting to some random (e.g., LST) values. Lastly, you didn't answer the following question:
|
|
I think I am fine with you browsing through the changed values. I have run the script from simtools-extra (gammasim/simtools-extra#1) to test and compare the parameters. They show good agreement now and I am confident that the prod5 / prod6 values in the new database are correct (have to do one more check on the site parameters) |
orelgueta
left a comment
orelgueta
left a comment
There was a problem hiding this comment.
Luckily I had to wait for cvmfs and decided to take a look while waiting. Two comments on the values that might be important.
| telescope_random_angle = 0.0 | ||
| telescope_random_error = 0.0 | ||
| telescope_transmission = 0.908661 0.0 0.0 0.0 0.0 0.0 | ||
| telescope_transmission = 0.908661 0.0154576 4.0 1.21166 |
There was a problem hiding this comment.
This was a bug in Prod5. It technically should have been
telescope_transmission = 0.908661,1,0.0154576,4.0,1.21166
However, the 1 was missing, so it was in fact interpreted during simulations as a constant value:
telescope_transmission = 0.908661
which is essentially what you had originally.
I therefore suggest that we either take what was actually simulated (i.e., revert this change), or we fix the Prod5 model in our database. I vote for the former since we want to have what was simulated in Prod5, even if it was wrong.
| telescope_random_angle = 0.0 | ||
| telescope_random_error = 0.0 | ||
| telescope_transmission = 0.908661 0.0 0.0 0.0 0.0 0.0 | ||
| telescope_transmission = 0.908661 0.0154576 4.0 1.21166 |
Analysis Details0 Issues
Coverage and Duplications
Project ID: gammasim_simtools_AY_ssha9WiFxsX-2oy_w |
|
Thanks again @orelgueta - fixed both issue. Let me know if this is fine and if we can go ahead and merge it into the *db-simulation-model-refactoring branch. |
GernotMaier
merged commit 715e1b5
into
db-simulation-model-refactoring
Important: this PR merges into db-simulation-model-refactoring
Corrections to the reference model (==values from the new model DB) to reproduce the original prod5/prod6 configurations as provided by sim_telarray.
Simplifies also the reading of names.get_simulation_software_name_from_parameter_name() (telescope/site parameters are not required).