LISTSERV 16.5 - HPS-SOFTWARE Archives

you forgot to put in the part where it tests if the TimeCorrEcalHits exists
in the lcio file, and uses it instead of EcalCalHits if the former exists.
I just made that change in my local copy.

  if(event->getCollection("TimeCorrEcalHits") != NULL)

  hits = (IMPL::LCCollectionVec*) event->getCollection("TimeCorrEcalHits");

  else

  hits = (IMPL::LCCollectionVec*) event->getCollection(hits_collection_name
);


After making this change, the DST i generated looks good now.  2 cluster
time difference looks like what i expect it to be, so this is good.

On Wed, Sep 21, 2016 at 9:31 PM, Nathan Baltzell <[log in to unmask]> wrote:

> I expect we'll definitely be reverting A to keep collection name unchanged.
> But still a useful test of B.
>
> -Nathan
>
>
> On Sep 21, 2016, at 9:14 PM, Maurik Holtrop <[log in to unmask]>
> wrote:
>
> > Hello Sebouh, all,
> >
> > Could you please try the updated version of hps-dst on the
> hps-dst_Issue44 branch on GitHub?
> >
> > (i.e. in your source directory: git pull; git checkout hps-dst_Issue44;
> then rebuild the dst-maker )
> >
> > That version should:
> >
> > A) Test to see of the TimeCorrEcalHits exist in the lcio file. If it
> does, use it, if it does not, fall back to EcalCalHits.
> > B) Use an std::pair<int,int> for the map key, with the first int the id0
> of the hit and the second int 10*getTime(). This should uniquely identify
> any hit.
> >
> > Please let me know if you still see any issue with that version.
> >
> > Tomorrow in the software meeting we will discuss if we are going to keep
> the “TimeCorrEcalHits” collection, but either way, I think the version I
> just put up should work.
> >
> > Best,
> >       Maurik
> >
> >
> >> On Sep 21, 2016, at 3:26 PM, Maurik Holtrop <[log in to unmask]>
> wrote:
> >>
> >> Yes. 1) They “should” be there. 2) If you want to check anything
> regarding ECAL you need all the hits, 3) The previous scheme would write
> shared hits twice.
> >> I am the one who needed this, as I was checking the clustering, looking
> for photons.
> >>
> >> Furthermore, the 2016 recon was changed. The new recon changed the name
> of the corrected Ecal hits to “TimeCorrEcalHits", so if you now want to
> analyze that data, whether with the DST or directly, you need to use this
> new name. I don’t think that is a good idea, I think it is confusing. I
> agree with Nathan that we should revert to the old name, “EcalCalHits”.
> >>
> >> Best,
> >>      Maurik
> >>
> >>
> >>
> >>> On Sep 21, 2016, at 3:08 PM, Omar Moreno <[log in to unmask]> wrote:
> >>>
> >>> Maurik, was there a reason that you decided to add all Ecal hits in an
> event to the DST?  Did someone request it or did you just feel like they
> needed to be in there?  The reason I ask is because if everyone is OK with
> just having hits associated with clusters, then we can just revert the code
> to what I originally had and just apply a fix for the repeated hits.  This
> can be easily done using a single map, without the need of using Cell
> ID's.  The original version of the code worked with both the 2015 and 2016
> recon so there will be no need to make changes to the LCIO recon.
> >>>
> >>> On Wed, Sep 21, 2016 at 11:43 AM, Maurik Holtrop <
> [log in to unmask]> wrote:
> >>> Hello Sebouh,
> >>>
> >>> Thank you for looking further into this. It is very useful debugging
> too.
> >>>
> >>> Unfortunately, I don’t think it is quite so simple as using the other
> collection. The Engrun2015 data does not have the TimeCorrEcalHits
> collection at all, it only has EcalCalHits. So if you switch the collection
> that is being used to TimeCorrEcalHits, you now break the older passes and
> all the 2015 data.
> >>>
> >>> Perhaps it is better for us to fix the Java code so that it does not
> introduce yet another collection of Ecal hits?
> >>>  
> >>>
> >>> Also, I thought that the CellID0 identified a hit uniquely, but as you
> point out, that was a misunderstanding. The SVT code uses a similar scheme
> to build the map to the hits.  I should look into that part of the code as
> well. Instead of using two maps, with a second “overflow” map, it would be
> better design to use a single map where each hit has a unique
> identification. I can look into what would work best. Perhaps some
> combination of the id0 and the time.
> >>>
> >>> When I wrote the SVT code, I made distributions to make sure things
> worked as expected.   Please don't touch that code ... 
> >>>
> >>> So please wait with a commit until we can figure out what is the best
> path forward.
> >>>
> >>> Best,
> >>>     Maurik
> >>>
> >>>> On Sep 21, 2016, at 2:16 PM, Sebouh Paul <[log in to unmask]>
> wrote:
> >>>>
> >>>> Two things I've found
> >>>> 1)  The collection name of the Ecal hits with corrections on the hit
> times is "TimeCorrEcalHits".   "EcalCalHits" does not have these
> corrections (see EcalTimeCorrectionDriver.java , which is the driver that
> makes these corrections)
> >>>> 2)  8.7% of the hits are duplicates (second hits) on the same
> crystal, so sometimes using the map<int, EcalHit*> will give us the wrong
> hit.
> >>>>
> >>>> The attached file contains corrections for both of those two
> problems.  Maurik and Omar, do I have your permission to check these
> changes into the git repository?
> >>>>
> >>>>
> >>>>
> >>>>
> >>>> On Wed, Sep 21, 2016 at 10:07 AM, Nathan Baltzell <[log in to unmask]>
> wrote:
> >>>> Hi Maurik,
> >>>>
> >>>> Well, I think we probably should instead change recon (probably only
> steering files
> >>>> are necessary) so the corrected hit collection name is back to what
> it used to be,
> >>>> instead of changing the dst-maker.  People (and the dst-maker
> previously) usually
> >>>> get to the hits via the clusters, so no one noticed a problem, but
> changing a
> >>>> collection's name is asking for trouble ...
> >>>>
> >>>> -Nathan
> >>>>
> >>>>
> >>>> On Sep 21, 2016, at 10:01, Maurik Holtrop <[log in to unmask]>
> wrote:
> >>>>
> >>>> > Hello Nathan, Sebouh, DST users,
> >>>> >
> >>>> > My bad. I apologize for incorrectly linking the ECAL hits from the
> DST to the EcalCalHits collection, which contrary to my understanding,
> seems to not contain the corrected hits. I will fix this problem as soon as
> I have figured out where the corrected hits are stored.
> >>>> >
> >>>> > I do think that the DST needs to contain the full collection of
> hits and not only the hits used by clusters, and certainly it should not
> store the same hit twice. Some some change in the DST is needed from the
> early behavior.
> >>>> >
> >>>> > I would appreciate it if someone could please point me to the
> documentation or the code that does the Ecal hit corrections, so I can
> figure out what it does and where it stores the output. See my email from
> yesterday about “CopyClusterCollectionDriver”.
> >>>> >
> >>>> > Thanks,
> >>>> >       Maurik
> >>>> >
> >>>> >
> >>>> >> On Sep 21, 2016, at 6:28 AM, Nathan Baltzell <[log in to unmask]>
> wrote:
> >>>> >>
> >>>> >> Hi All,
> >>>> >>
> >>>> >> It looks like at least 2 things happened that are contributing to
> what
> >>>> >> Sebouh sees:
> >>>> >>
> >>>> >> 1. Prior to the release 3-9 used for 2016 pass0, ecal recon was
> modified
> >>>> >> to have a separate driver do hit time corrections (a first step in
> the
> >>>> >> suggested code cleanup from a while a ago).  It makes a hit
> collection
> >>>> >> with a *new* name used by clustering.
> >>>> >>
> >>>> >> 2. Something was late changed in how dst-maker reads ecal hits.
> >>>> >>
> >>>> >> Somehow, after #1 but before #2 dsts were still ok.  Seems #1, for
> backward
> >>>> >> compatibility, should be changed to output hit collection of same
> name as
> >>>> >> before, or modify the input hit collection in-place.
> >>>> >>
> >>>> >> -Nathan
> >>>> >>
> >>>> >>
> >>>> >>
> >>>> >>
> >>>> >> On Sep 20, 2016, at 4:09 PM, Maurik Holtrop <
> [log in to unmask]> wrote:
> >>>> >>
> >>>> >>> Hello ECAL folks,
> >>>> >>>
> >>>> >>> I am mightily confused about our Ecal collections in LCIO.
> Without reading a ton of code, could someone please help me sort some
> things out here?
> >>>> >>>
> >>>> >>> According to the Confluence page:
> >>>> >>>
> >>>> >>> EcalClustersCorr    Cluster CopyClusterCollectionDriver
> >>>> >>> reconstructed ECal clusters with corrected energies
> >>>> >>>
> >>>> >>> This is very confusing. The driver “CopyClusterCollectionDriver”
> is supposed to just copy a collection, not also make corrections to the
> timing and energy, and indeed, it doesn’t seem to, so then EcalClustersCorr
> and EcalClusters are identical, or not? If they are the same, why? If not
> the same, how?
> >>>> >>>
> >>>> >>> I just confirmed that the hits that written in the
> “EcalClustersCorr” collection are NOT the same as those in EcalCalHits,
> when these hits have the same ID. So that puzzles me. Where are those hits
> changed? What collection are the corrected hits in?
> >>>> >>>
> >>>> >>> The “ReconClusterer” (nicely commented and quite readable code,
> thank you) that made the EcalClusters collection uses the EcalCalHits and
> does not appear to make any corrections to them. Am I wrong?
> >>>> >>>
> >>>> >>> Any help here would be appreciated. We can update the Confluence
> page with the answers.
> >>>> >>>
> >>>> >>> Best,
> >>>> >>>     Maurik
> >>>> >>>
> >>>> >>>
> >>>> >>>> On Sep 20, 2016, at 2:54 PM, Maurik Holtrop <
> [log in to unmask]> wrote:
> >>>> >>>>
> >>>> >>>> Hello Omar and Sebouh,
> >>>> >>>>
> >>>> >>>> I will look into the issue of the hits timing, and directly
> compare the various collections we have in LCIO. Give me a little time
> though.
> >>>> >>>>
> >>>> >>>> In the LCIO files, I can find the EcalReadoutHit (sounds like it
> is “raw” hit from Ecal) and hte EcalCalHit. According to the driver
> “EcalRawConverterDriver” the EcalCalHit is the output (and EcalReadoutHit
> the input), so it seems to me that is the one to use. Also, on Confluence:
> >>>> >>>>
> >>>> >>>> EcalCalHits        CalorimeterHit  EcalRawConverterDriver
> calibrated ECal Hits
> >>>> >>>>
> >>>> >>>> Documentation is pretty sparse though, so correct me if I am
> wrong and need to use a different collection. I find the large number of
> collection more than a little confusing at times.
> >>>> >>>>
> >>>> >>>> Best,
> >>>> >>>>    Maurik
> >>>> >>>>
> >>>> >>>>
> >>>> >>>>
> >>>> >>>>> On Sep 20, 2016, at 1:51 PM, Omar Moreno <[log in to unmask]>
> wrote:
> >>>> >>>>>
> >>>> >>>>>
> >>>> >>>>> Originally, as Maurik said, the DST maker only wrote Ecal hits
> associated with a corrected Ecal cluster (i.e.
> cluster->getCalorimeterHits();)  With Maurik's changes, all Ecal hits
> written to the DST now come from the collection "EcalCalHits".  The cluster
> time comes from the seed hit of the cluster so if there was a correction
> applied to the seed that wasn't propagated to the original EcalCalHits
> collection, then this might be the issue.  This might be the reason why I
> didn't write all of the Ecal hits to the DST maker to begin with but I
> don't remember.
> >>>> >>>>>
> >>>> >>>>> Maurik,  did you look at distributions of Ecal clusters
> variables before and after your changes in order to make sure that there
> wasn't any changes?  If not, then we should revert all of your changes and
> do these checks.
> >>>> >>>>>
> >>>> >>>>>
> >>>> >>>>> On Sep 20, 2016 9:59 AM, "Sebouh Paul" <[log in to unmask]>
> wrote:
> >>>> >>>>> To get technical, I did not look at the recon LCIO files
> directly, but rather generated DQM from them.  And the DQM did not look
> different between the two files.  Now what Nathan had suggested was that
> possibly the DST maker is reading the hits from a different collection than
> it was reading from before, for instance uncallibrated hits instead of
> callibrated hits?
> >>>> >>>>>
> >>>> >>>>> On Tue, Sep 20, 2016 at 11:21 AM, Omar Moreno <
> [log in to unmask]> wrote:
> >>>> >>>>> Just to make sure, this behavior isn't observed looking at the
> recon LCIO files correct?
> >>>> >>>>>
> >>>> >>>>>
> >>>> >>>>> On Sep 20, 2016 7:59 AM, "Sebouh Paul" <[log in to unmask]>
> wrote:
> >>>> >>>>> Without pulse fitting, or the mode7 emulation, all of the hit
> times would be at times that are multiples of 4 ns, corresponding to the
> time of the first sample crossing the threshold.  However, we use a pulse
> fitting algorithm, which improves our resolution on timing for the hit, as
> well as time-walk corrections.  If a fit fails the pulse fitting and the
> mode-7 emulation, then we use the time of the first sample that crosses the
> threshold.   In the "bad files" we see a lot of low energy hits with
> multiples of 4ns times, which probably failed both these two methods of
> time reconstruction.
> >>>> >>>>>
> >>>> >>>>> There is also another thing in here I forgot to mention:  there
> is also a shift in the timing window of when multiple hits are, between the
> "bad" files and the "good" files. What i mean is, you'll see a peak in hit
> time, corresponding to the trigger window.  This peak is shifted between
> files generated using the old version of the  DST and the new version.
> >>>> >>>>>
> >>>> >>>>> On Tue, Sep 20, 2016 at 10:47 AM, Maurik Holtrop <
> [log in to unmask]> wrote:
> >>>> >>>>> Hello Sebouh,
> >>>> >>>>>
> >>>> >>>>> There was a bug in the DST maker where it would only write ECAL
> hits that were part of a cluster, and if a hit appeared in two clusters, it
> would write that hit twice. This is not the correct behavior. I fixed this
> in Issue44 (https://github.com/JeffersonLab/hps-dst/issues/44) and then
> merged to master.
> >>>> >>>>>
> >>>> >>>>> So, #1, is because now the unrelated and out of time hits are
> also in the DST.
> >>>> >>>>> I don’t think this explains #2.
> >>>> >>>>> #3 Since the FADC samples at 250Mhz, I would expect all ECAL
> hit times to be at multiples of 4ns, but perhaps I am missing something?
> >>>> >>>>>
> >>>> >>>>> Best,
> >>>> >>>>>   Maurik
> >>>> >>>>>
> >>>> >>>>>> On Sep 20, 2016, at 9:37 AM, Sebouh Paul <
> [log in to unmask]> wrote:
> >>>> >>>>>>
> >>>> >>>>>> Has anyone made modifications to the DST maker since pass0
> began in June 6? Nathan ran recon and DST maker on some files at the
> beginning of August, and there are some problems with the DST files
> produced:
> >>>> >>>>>>
> >>>> >>>>>> 1)  1.5x more ecal hits per event than in DST files generated
> in pass0.
> >>>> >>>>>> 2)  two cluster time difference has a much larger sigma than
> usual.
> >>>> >>>>>> 3)  many ecal hits have hit times that are exact multiples of
> 4 ns.
> >>>> >>>>>>
> >>>> >>>>>> I ran DQM on the corresponding LCIO files, and saw none of
> these effects, so the problem must be in the DST maker.
> >>>> >>>>>>
> >>>> >>>>>> thanks.
> >>>> >>>>>>
> >>>> >>>>>> Use REPLY-ALL to reply to list
> >>>> >>>>>>
> >>>> >>>>>> To unsubscribe from the HPS-SOFTWARE list, click the following
> link:
> >>>> >>>>>> https://listserv.slac.stanford.edu/cgi-bin/wa?
> SUBED1=HPS-SOFTWARE&A=1
> >>>> >>>>>>
> >>>> >>>>>
> >>>> >>>>>
> >>>> >>>>> Use REPLY-ALL to reply to list
> >>>> >>>>>
> >>>> >>>>> To unsubscribe from the HPS-SOFTWARE list, click the following
> link:
> >>>> >>>>> https://listserv.slac.stanford.edu/cgi-bin/wa?
> SUBED1=HPS-SOFTWARE&A=1
> >>>> >>>>>
> >>>> >>>>>
> >>>> >>>>>
> >>>> >>>>> Use REPLY-ALL to reply to list
> >>>> >>>>>
> >>>> >>>>> To unsubscribe from the HPS-SOFTWARE list, click the following
> link:
> >>>> >>>>> https://listserv.slac.stanford.edu/cgi-bin/wa?
> SUBED1=HPS-SOFTWARE&A=1
> >>>> >>>>>
> >>>> >>>>>
> >>>> >>>>
> >>>> >>>
> >>>> >>>
> >>>> >>> Use REPLY-ALL to reply to list
> >>>> >>>
> >>>> >>> To unsubscribe from the HPS-SOFTWARE list, click the following
> link:
> >>>> >>> https://listserv.slac.stanford.edu/cgi-bin/wa?
> SUBED1=HPS-SOFTWARE&A=1
> >>>>
> >>>>
> >>>> <EcalDataWriter.cxx>
> >>>
> >>>
> >>> Use REPLY-ALL to reply to list
> >>>
> >>> To unsubscribe from the HPS-SOFTWARE list, click the following link:
> >>> https://listserv.slac.stanford.edu/cgi-bin/wa?SUBED1=HPS-SOFTWARE&A=1
> >>>
> >>>
> >>
> >>
> >> Use REPLY-ALL to reply to list
> >>
> >> To unsubscribe from the HPS-SOFTWARE list, click the following link:
> >> https://listserv.slac.stanford.edu/cgi-bin/wa?SUBED1=HPS-SOFTWARE&A=1
> >>
> >
> >
> > Use REPLY-ALL to reply to list
> >
> > To unsubscribe from the HPS-SOFTWARE list, click the following link:
> > https://listserv.slac.stanford.edu/cgi-bin/wa?SUBED1=HPS-SOFTWARE&A=1
> >
>
>

########################################################################
Use REPLY-ALL to reply to list

To unsubscribe from the HPS-SOFTWARE list, click the following link:
https://listserv.slac.stanford.edu/cgi-bin/wa?SUBED1=HPS-SOFTWARE&A=1