https://cta-redmine.irap.omp.eu/https://cta-redmine.irap.omp.eu/favicon.ico?14312453732021-05-11T15:32:43ZCTA IRAP Project Gatewayctools - Feature #3668: add minimal event counts to cssenshttps://cta-redmine.irap.omp.eu/issues/3668?journal_id=163272021-05-11T15:32:43ZSadeh Iftachiftach.sadeh@desy.de
<ul></ul><p>Implemented here:<br /><a class="external" href="https://cta-gitlab.irap.omp.eu/sadeh/ctools/tree/cssens_source_min_event_cuts">https://cta-gitlab.irap.omp.eu/sadeh/ctools/tree/cssens_source_min_event_cuts</a></p> ctools - Feature #3668: add minimal event counts to cssenshttps://cta-redmine.irap.omp.eu/issues/3668?journal_id=163552021-05-14T15:47:38ZSadeh Iftachiftach.sadeh@desy.de
<ul></ul><p>And yet another new feature in this pull request:<br />- in case edisp is set, only start using it in practice once the test flux is close to convergence.<br />This saves considerable CPU time.</p>
<p>Specifically, the modification follows (as part of the source flux calculation loop):<br /><pre>
# if the flux has converged, check if the original value of
# edisp was set, and is not what has fo far been used
if edisp_orig and not self['edisp'].boolean():
# set edisp on and continue the calculation
self['edisp'] = True
value = 'set edisp on after initial convergence withouth it'
self._log_value(gammalib.TERSE, 'Converged result', value)
else:
# finish the calculation after convergence
break
</pre><br />This comes instead of directly breaking at this point of the calculation (after convergence).</p> ctools - Feature #3668: add minimal event counts to cssenshttps://cta-redmine.irap.omp.eu/issues/3668?journal_id=163832021-05-19T15:36:12ZKnödlseder Jürgenjurgen.knodlseder@irap.omp.eu
<ul><li><strong>Target version</strong> set to <i>2.0.0</i></li></ul><p>Thanks for proposing these changes. I will look to them as soon as possible.</p>
<p>I justed wanted to make you aware that I worked the last days on a old features which is switching all ASCII file output to FITS files, see <a href="https://cta-redmine.irap.omp.eu/issues/1707" class="issue tracker-5 status-5 priority-5 priority-high3 closed" title="Review ASCII output of cscripts and ctools (Closed)">#1707</a>. This will affect <code>cssens</code>, hence I will first finished this change and then merge in your changes.</p> ctools - Feature #3668: add minimal event counts to cssenshttps://cta-redmine.irap.omp.eu/issues/3668?journal_id=163842021-05-19T15:37:21ZKnödlseder Jürgenjurgen.knodlseder@irap.omp.eu
<ul><li><strong>Duplicates</strong> <i><a href="/issues/2130" class="issue tracker-2 status-5 priority-4 priority-default closed">Feature #2130</a>: Incorporate minimum counts to sensitivity calculation</i> added</li></ul> ctools - Feature #3668: add minimal event counts to cssenshttps://cta-redmine.irap.omp.eu/issues/3668?journal_id=163862021-05-19T15:38:20ZKnödlseder Jürgenjurgen.knodlseder@irap.omp.eu
<ul><li><strong>Status</strong> changed from <i>In Progress</i> to <i>Closed</i></li></ul><p>A duplicated feature was created with issue <a href="https://cta-redmine.irap.omp.eu/issues/3668" class="issue tracker-2 status-5 priority-4 priority-default closed" title="add minimal event counts to cssens (Closed)">#3668</a>. Since some code changes were done on this feature I close this issue now.</p> ctools - Feature #3668: add minimal event counts to cssenshttps://cta-redmine.irap.omp.eu/issues/3668?journal_id=163882021-05-19T16:00:02ZSadeh Iftachiftach.sadeh@desy.de
<ul><li><strong>% Done</strong> changed from <i>70</i> to <i>100</i></li></ul><p>Hi Jürgen,</p>
<p>I’m not sure I follow. Which duplicated issue are you referring to?<br />I’d like to keep track.</p>
<p>In any case, once you go through your other updates, please check out<br /><a class="external" href="https://cta-gitlab.irap.omp.eu/sadeh/ctools/tree/cssens_source_min_event_cuts">https://cta-gitlab.irap.omp.eu/sadeh/ctools/tree/cssens_source_min_event_cuts</a></p>
<p>To summarize all changes:<br />1. Add optional absolute minimal event count on test source<br />2. Add optional minimal threshold of test source counts with respect to non-source counts<br />3. Numerical stability updates for source prefactor and for the _predict_flux regression function<br />4. Avoid extremely slow unnecessary energy dispersion calculations<br />5. Add optional random seed initialization for event generation</p>
<p>I know this is a lot for a single pull request. I hope you can integrate the content.</p> ctools - Feature #3668: add minimal event counts to cssenshttps://cta-redmine.irap.omp.eu/issues/3668?journal_id=164002021-05-19T20:57:13ZKnödlseder Jürgenjurgen.knodlseder@irap.omp.eu
<ul></ul><p><a href="https://cta-redmine.irap.omp.eu/users/sadeh" class="user">Sadeh Iftach</a> wrote:</p>
<blockquote>
<p>Hi Jürgen,</p>
<p>I’m not sure I follow. Which duplicated issue are you referring to?<br />I’d like to keep track.</p>
</blockquote>
<p>This was feature <a href="https://cta-redmine.irap.omp.eu/issues/2130" class="issue tracker-2 status-5 priority-4 priority-default closed" title="Incorporate minimum counts to sensitivity calculation (Closed)">#2130</a> that was there since a while asking to incorporate minimum counts into the sensitivity calculation. Since no one was working on that I closed this feature now and keep this one alive.</p>
<blockquote>
<p>In any case, once you go through your other updates, please check out<br /><a class="external" href="https://cta-gitlab.irap.omp.eu/sadeh/ctools/tree/cssens_source_min_event_cuts">https://cta-gitlab.irap.omp.eu/sadeh/ctools/tree/cssens_source_min_event_cuts</a></p>
<p>To summarize all changes:<br />1. Add optional absolute minimal event count on test source<br />2. Add optional minimal threshold of test source counts with respect to non-source counts<br />3. Numerical stability updates for source prefactor and for the _predict_flux regression function<br />4. Avoid extremely slow unnecessary energy dispersion calculations<br />5. Add optional random seed initialization for event generation</p>
<p>I know this is a lot for a single pull request. I hope you can integrate the content.</p>
</blockquote>
<p>No worries, I will go over the changes. I probably will propose shorter names for the new parameters as they are rather long compared to typical ctools names and a bit cryptic.</p> ctools - Feature #3668: add minimal event counts to cssenshttps://cta-redmine.irap.omp.eu/issues/3668?journal_id=164022021-05-20T06:50:01ZKnödlseder Jürgenjurgen.knodlseder@irap.omp.eu
<ul><li><strong>Status</strong> changed from <i>Closed</i> to <i>Pull request</i></li><li><strong>% Done</strong> changed from <i>100</i> to <i>80</i></li></ul><p>I had a first look over the code. I have seen that you use the <code>statistics</code> Python module that is only available from Python 3.4 on, which breaks the compatibility of ctools with older Python versions.</p>
<p>I see that you use the module only for the <code>median()</code> function, hence we should implement this function natively in the code so that we do not need the dependency.</p> ctools - Feature #3668: add minimal event counts to cssenshttps://cta-redmine.irap.omp.eu/issues/3668?journal_id=164032021-05-20T06:51:02ZKnödlseder Jürgenjurgen.knodlseder@irap.omp.eu
<ul></ul><p><a href="https://cta-redmine.irap.omp.eu/users/jknodlseder" class="user">Knödlseder Jürgen</a> wrote:</p>
<blockquote>
<p>A duplicated feature was created with issue <a href="https://cta-redmine.irap.omp.eu/issues/3668" class="issue tracker-2 status-5 priority-4 priority-default closed" title="add minimal event counts to cssens (Closed)">#3668</a>. Since some code changes were done on this feature I close this issue now.</p>
</blockquote>
<p>Closing was in fact not my intention here, sorry for the confusion. I closed the duplicated feature.</p> ctools - Feature #3668: add minimal event counts to cssenshttps://cta-redmine.irap.omp.eu/issues/3668?journal_id=164042021-05-20T07:14:58ZKnödlseder Jürgenjurgen.knodlseder@irap.omp.eu
<ul></ul><p>I implemented the following code to replace the median function:<br /><pre><code class="python syntaxhl"><span class="CodeRay"> <span class="keyword">def</span> <span class="function">_median</span>(<span class="predefined-constant">self</span>, array):
<span class="docstring"><span class="delimiter">"""</span><span class="content"> </span><span class="content">
</span><span class="content"> Compute median value for an array</span><span class="content">
</span><span class="content">
</span><span class="content"> Parameters</span><span class="content">
</span><span class="content"> ----------</span><span class="content">
</span><span class="content"> array : list of floats</span><span class="content">
</span><span class="content"> Array</span><span class="content">
</span><span class="content">
</span><span class="content"> Returns</span><span class="content">
</span><span class="content"> -------</span><span class="content">
</span><span class="content"> median : float</span><span class="content">
</span><span class="content"> Median value of array</span><span class="content">
</span><span class="content"> </span><span class="delimiter">"""</span></span>
<span class="comment"># Initialise median</span>
median = <span class="float">0.0</span>
<span class="comment"># Get number of elements in array</span>
n = <span class="predefined">len</span>(array)
<span class="comment"># Continue only if there are elements in the array</span>
<span class="keyword">if</span> n > <span class="integer">0</span>:
<span class="comment"># Sort the array</span>
wrk_array = <span class="predefined">sorted</span>(array)
<span class="comment"># Get median</span>
<span class="keyword">if</span> n % <span class="integer">2</span> != <span class="integer">0</span>:
median = wrk_array[<span class="predefined">int</span>(n/<span class="integer">2</span>)]
<span class="keyword">else</span>:
median = (wrk_array[<span class="predefined">int</span>((n-<span class="integer">1</span>)/<span class="integer">2</span>)] + wrk_array[<span class="predefined">int</span>(n/<span class="integer">2</span>)]) / <span class="float">2.0</span>
<span class="comment"># Return median</span>
<span class="keyword">return</span> median
</span></code></pre><br />Testing for an empty array gives:<br /><pre>
0.0
Traceback (most recent call last):
File "./test.py", line 19, in <module>
print(statistics.median(array))
File "/Users/jurgen/anaconda3/envs/ctools-1.7.4/lib/python3.8/statistics.py", line 430, in median
raise StatisticsError("no median for empty data")
statistics.StatisticsError: no median for empty data
</pre>where the first result is my code and the second the code from the statistics module.</p>
<p>Here a few tests with different arrays:</p>
<table>
<tr>
<td> <strong>array</strong> </td>
<td> <strong>cssens._median</strong> </td>
<td> <strong>statistics.median</strong> </td>
</tr>
<tr>
<td> [1] </td>
<td> 1 </td>
<td> 1 </td>
</tr>
<tr>
<td> [2,1] </td>
<td> 1.5 </td>
<td> 1.5 </td>
</tr>
<tr>
<td> [2.0,1.0] </td>
<td> 1.5 </td>
<td> 1.5 </td>
</tr>
<tr>
<td> [2.0,3.0,1.0,5.0,1.0] </td>
<td> 2.0 </td>
<td> 2.0 </td>
</tr>
<tr>
<td> [1,2,3,4, 10,11,12,13,9,8,20,21] </td>
<td> 9.5 </td>
<td> 9.5 </td>
</tr>
</table>
<p>The code seems to work as expected.</p> ctools - Feature #3668: add minimal event counts to cssenshttps://cta-redmine.irap.omp.eu/issues/3668?journal_id=164052021-05-20T08:17:12ZTibaldo Luigiluigi.tibaldo@irap.omp.eu
<ul></ul><p>Median is a very low-level function. Shouldn’t it be implemented somewhere else so that it can be used by other ctools/cscripts in the future? For example GMath or GNdarray?</p> ctools - Feature #3668: add minimal event counts to cssenshttps://cta-redmine.irap.omp.eu/issues/3668?journal_id=164072021-05-20T08:52:41ZKnödlseder Jürgenjurgen.knodlseder@irap.omp.eu
<ul><li><strong>Related to</strong> <i><a href="/issues/3693" class="issue tracker-6 status-1 priority-4 priority-default">Action #3693</a>: Add statistics methods to GNdarray</i> added</li></ul> ctools - Feature #3668: add minimal event counts to cssenshttps://cta-redmine.irap.omp.eu/issues/3668?journal_id=164082021-05-20T08:53:02ZKnödlseder Jürgenjurgen.knodlseder@irap.omp.eu
<ul></ul><p><a href="https://cta-redmine.irap.omp.eu/users/ltibaldo" class="user">Tibaldo Luigi</a> wrote:</p>
<blockquote>
<p>Median is a very low-level function. Shouldn’t it be implemented somewhere else so that it can be used by other ctools/cscripts in the future? For example GMath or GNdarray?</p>
</blockquote>
<p>Fully agree. The best place would probably be the <code>GNdarray</code> class, which should then also get <code>mean()</code> and <code>std()</code> methods. I created an issue for that: <a href="https://cta-redmine.irap.omp.eu/issues/3693" class="issue tracker-6 status-1 priority-4 priority-default" title="Add statistics methods to GNdarray (New)">#3693</a>.</p> ctools - Feature #3668: add minimal event counts to cssenshttps://cta-redmine.irap.omp.eu/issues/3668?journal_id=164122021-05-20T10:38:51ZSadeh Iftachiftach.sadeh@desy.de
<ul></ul><p>Thanks, Jürgen.</p>
<p>Of course, please feel free to make any changes to variable names etc. to fit the ctools conventions.<br />At the moment I understand that I have no action items for myself, so please let me know if you’d like me to modify any code.</p>
<p>An outstanding open question is the choice of default settings.<br />In my opinion, the minimal 10 evt source count can be the new default (rather than the current default of 0). This complies with what everyone expects with respect to the “official” cta results (produced with EventDisplay), and shown on the public website.</p>
<p>The second condition on the background counts is not relevant in most cases (except for very long exposures). In my opinion it is also less rigorously defined, especially considering the full-enclosure IRFs.<br />I’m agnostic about wether or not this one should be turned on by default.</p> ctools - Feature #3668: add minimal event counts to cssenshttps://cta-redmine.irap.omp.eu/issues/3668?journal_id=164152021-05-20T11:12:50ZKnödlseder Jürgenjurgen.knodlseder@irap.omp.eu
<ul></ul><p>I agree that setting the default value to 10 is maybe what we want to do, although Gernot mentioned recently that these criteria may be revisited.</p>
<p>I’m basically done with the code integration, I did a bit of refactoring (introducing a method <code>cssens._simulate_events</code> that avoids having a loop over source and background in <code>cssens._sim_evt_excess</code>), modified slightly the logging (so that values all fit in the expected length and that detailed information is only written when chatter is in <code>EXPLICIT</code>) and changed the parameters to shorter names:<br /><pre>
mincounts, i, h, 0,,, "Minimum number of source counts"
bkgexcess, r, h, 0.0,,, "Background uncertainty fraction"
bkgrad, r, h, 0.33,,, "Background radius (deg)"
seed, i, h, 1,,, "Initial random number generator seed"
</pre></p>
<p>I also updated the reference manual to add the new parameters. The documentation of all tools is in <code>doc/source/users/reference_manual/</code> which is also used to build the help text that you get with <code>cssens --help</code>.</p> ctools - Feature #3668: add minimal event counts to cssenshttps://cta-redmine.irap.omp.eu/issues/3668?journal_id=164222021-05-20T12:20:05ZKnödlseder Jürgenjurgen.knodlseder@irap.omp.eu
<ul><li><strong>Status</strong> changed from <i>Pull request</i> to <i>Closed</i></li><li><strong>% Done</strong> changed from <i>80</i> to <i>100</i></li></ul><p>I merged the code into <code>devel</code>. Close the issue now.</p> ctools - Feature #3668: add minimal event counts to cssenshttps://cta-redmine.irap.omp.eu/issues/3668?journal_id=164282021-05-20T14:18:11ZSadeh Iftachiftach.sadeh@desy.de
<ul></ul><p>Great!</p>
<p>One final note, not really critical...<br />I tried to explicitly use “non-source” rather than “background” terminology for <br /><pre>
bkgexcess, r, h, 0.0,,, "Background uncertainty fraction"
bkgrad, r, h, 0.33,,, "Background radius (deg)"
</pre><br />This may be a bit misleading for the user, since the cut is done based on all background + [not test source]-sources in the RoI (not exclusively the IRF background).</p> ctools - Feature #3668: add minimal event counts to cssenshttps://cta-redmine.irap.omp.eu/issues/3668?journal_id=164292021-05-20T14:23:49ZKnödlseder Jürgenjurgen.knodlseder@irap.omp.eu
<ul></ul><p>I understand, but I don’t think that a user will understand what <code>non-source</code> means. We may clarify in the documentation that “background” means everything else than the source (a bit like for the DM community).</p> ctools - Feature #3668: add minimal event counts to cssenshttps://cta-redmine.irap.omp.eu/issues/3668?journal_id=164302021-05-20T14:25:17ZSadeh Iftachiftach.sadeh@desy.de
<ul></ul><p>Great.<br />Thanks again for the quick integration of this pull request.</p>