-
-
Notifications
You must be signed in to change notification settings - Fork 410
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
Virtual Spectrum Zero #445
Comments
Seems to be related to the virtual packet logging scheme introduced in PR #433. When running with Tardis complied with the flag This is clearly not what we intended with PR #433. Here, only the additional memory intensive virtual_packet diagnostic arrays should be activated or de-activated. The virtual spectrum, however, should be calculated in any case (correct me if I am wrong, @wkerzendorf, @orbitfold) I'll have a closer look at this shortly and will try to fix it. |
If I understand it correctly then this is "correct" behavior. It should not be stored when --with-vpacket-logging is not enabled. Virtual packets are still used, just the spectrum is not stored. I could have gotten this wrong though. |
The only reason to use virtual packets is to increase the signal-to-noise in the spectrum. So, there would be not much point to use virtual packets, which slow down the calculation, and then not benefit from the only advantage that they offer, namely decreasing the MC noise in the spectrum. So, whenever virtual packets are active in the simulation, we definitely want to calculate/store the spectrum. However, we not necessary need always to keep track of all the interaction details of the virtual packets. That said, your switch, which is very useful, should only activate and deactivate the storage of arrays such as
but should not interfere with calculating and storing the virtual spectrum. Again, correct me if I am wrong, @ssim, @wkerzendorf. |
Well the spectrum is still calculated and stored. The reason for storing the spectrum for virtual packets alone was to use the information in uncertainty calculations. It wasn't stored before we needed it this summer, I believe. Which is also why tests didn't fail. The full spectrum test uses virtual packets. It would have failed if they stopped working. |
Seems to be not the case, as the following comparisons show: Both plots were produced with the following instructions:
|
Found the problem, I think - see the inline note I just added to the PR #433 |
The virtual spectrum is calculated in simulation/base.py lines 318-323, using the Whether this array is populated (cmontecarlo.c, lines 451-456) is currently subject to a large ifdef block and is thus only performed if vpacket loggin is active. I'll open a PR to fix this shortly. |
I noticed this while investigating issue #191: using the current upstream/master 34f667a, Tardis does not produce a virtual spectrum any more. The following model attributes are all arrays populated exclusively with zeros:
This is a serious issue. It is also worrisome that none of our automated tests picked this one up. @wkerzendorf: we should include a
test_virtual_spectrum
routine into thetest_tardis_full
script.The text was updated successfully, but these errors were encountered: