CORSIKA support and small CRY improvements#35
Merged
Conversation
added 9 commits
October 19, 2019 14:13
kutschke
suggested changes
Oct 22, 2019
kutschke
left a comment
kutschke
left a comment
Contributor
There was a problem hiding this comment.
Only append to these IDs. Never insert new values. This changes the meaning of identifiers already written to existing files.
kutschke
suggested changes
Oct 22, 2019
kutschke
left a comment
kutschke
left a comment
Contributor
There was a problem hiding this comment.
We have a standard prolog definition for the geometry service. Please use that. By doing so people can change the default configuration of the geometry service without you needing to maintain .fcl files.
Contributor
Author
|
I fixed the GenId.hh file and the fcl geometry statement |
Contributor
|
Hi Roberto, I added a lot of small picky comments. I also want to say that in the big picture this looks really good. I also find your coding style and commenting style very easy to follow. I realize now that I should have sent this first but the github interface makes it natural to send it last! Rob |
kutschke
reviewed
Oct 23, 2019
kutschke
left a comment
kutschke
left a comment
Contributor
There was a problem hiding this comment.
Hi Roberto,
We need to discuss two things:
1)
How do you plan to package CORSIKA and all of the other software in it's stack? UPS? You were on uBooNE right? Do they have everything packaged as UPS products?
We should discuss whether or not the file format you are using is the right long term choice for Mu2e. It was a great choice for getting started but we now need to think about how we will structure production runs for the next decade.
I won't have time to tape part in this until next week. These issues should not slow down this pull request but we should address them sooner rather than later.
Rob
|
|
||
| namespace mu2e { | ||
|
|
||
| class CorsikaEventGenerator : public art::EDProducer { |
Contributor
There was a problem hiding this comment.
I prefer that you make this a source module not a producer. Among other things this lets you end gracefully the job if you run out of input events.
Contributor
Author
There was a problem hiding this comment.
I am not sure I understand what you mean by source module, could you please clarify?
Contributor
There was a problem hiding this comment.
Instead of inheriting from EDAnalyzer or EDProducer or EDFilter you do something else ( the rules are a little more complicated). Then you specify your module in the source : {} table of the fcl file, instead of EmptyEvent or RootInput. The big win is that if you run out of events then you can gracefully shutdown the job. If you implement jobs as EmptySource and a producer module then, if you run out of events, you have to throw an exception, which works but is not very user friendly.
Andrei has experience writing source modules. You can anexamples in:
Sources/src/FromEMFMARSFileWeighted_source.cc
The interface to art is not via inheritance. Instead you write a "detail class" then the actual source module is the instantiation of a source module template using your detail class as the template argument. It's just a different way for you to provide callbacks that are called by the art code at the right time with the right arguments.
Contributor
Author
There was a problem hiding this comment.
I am afraid this is beyond my C++ competencies... I left it as it was, also because it is the same approach of CRY, but I am happy to change it with a little of help from more expert people. Also, at the moment the module can't run out of events because if it reaches the last binary file it starts again from the first one, shifting the position of the cosmic rays by a random amount.
Contributor
Author
There was a problem hiding this comment.
Ok, I talked with @brownd1978 and I see your point now. I will work on it but it will take a couple of days since I am not very familiar with it. At the moment I was avoiding the "end of the file" problem by starting again from the first file and moving the particle by a random amount (which is basically resampling) but I see it's better to separate the two logics.
Contributor
Author
There was a problem hiding this comment.
I wrote the Source module, however it wasn't as straightforward as I thought: the source module could access the geometry only after the first event had been processed, so I couldn't extrapolate the cosmic rays to the world borders. In order to avoid ugly solutions (such as storing a first empty event) I had to write also a producer which takes as input the output of the Source module and does the backwards extrapolation. I compared the outputs before and after these changes and they are the same.
Contributor
Author
|
Regarding the high-level issues:
|
Contributor
|
On Oct 23, 2019, at 1:11 PM, Stefano Roberto Soleti ***@***.***> wrote:
Regarding the high-level issues:
• I don't think we need to package CORSIKA as a UPS product. Right now I just create a tarball of CORSIKA+FLUKA and send a script with jobsub and it works just fine. That's the way we used in MicroBooNE. The art module doesn't require CORSIKA because the output can be read without any CORSIKA library.
The uBooNE solution does not have as strong an audit trail as I want Mu2e to have. Sending around tarballs is, right now, much less efficient than running from cvmfs. It will soon become only a little
less efficient but it will still be less efficienct.
• About the file format: I don't use sqlite anymore, I read from the CORSIKA binary output directly and it's much faster (~x100). I don't think we need to store the CORSIKA binaries. My workflow idea is:
• Generate the CORSIKA binary output
• Read the CORSIKA binary file and produce the GenParticleCollection.
• Delete the CORSIKA binary output (you would still have the CORSIKA configuration in case you really want to check the binary in the future).
Since CORSIKA binaries are very large (~4GB for 10000 showers) I think this is the best solution. However I am not sure technically how to implement it.
I am glad you got rid of the db. I expected it would turn out to be noticeably slower.
It won't be hard to implement the workflow you describe - we have long believed that some day we would need to support that workflow.
However for development and debugging I think that we want to retain the ability to keep the CORSIKA binaries.
… —
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
Contributor
|
Hi Roberto,
Adding Andrei and Ray to this.
What do people think about this solution? Is this an example of good separation of function - it keeps
the source module clean? Or is it an artificial limitation of our geometry system being out of date and therefore
causing artificial module pollution. I can see arguments on both sides but I think it's the latter.
In our present design:
- you get *nominal* geometry from the geeometry service; it is fixed for a full art run.
- you get aligned geometry from the ProditionsService
- many geometry elements will only be in the GeometryService
- The dimensions of the world volume will only be in the GeometryService; they are not run dependent
but they may be production campaign dependent.
I believe that it's time to move the initialization of the GeometryService to BeginJob. We would leave
ProditionsService as it is - it initializes objects on demand and requires a SubRunID in order to do its job.
In this way code that needs only *nominal* Geometry can run anytime after BeginJob. This would allow
Roberto to return to a single module solution.
Any comments on this? Do we need to broaden the discussion?
Rob
… On Oct 28, 2019, at 6:41 PM, Stefano Roberto Soleti ***@***.***> wrote:
@soleti commented on this pull request.
In EventGenerator/src/CORSIKAEventGenerator_module.cc:
> +#include "art/Framework/Principal/Handle.h"
+#include "fhiclcpp/ParameterSet.h"
+#include "messagefacility/MessageLogger/MessageLogger.h"
+
+// C++ includes.
+#include <iostream>
+#include <sstream>
+#include <string>
+#include <vector>
+#include <memory>
+
+#include "EventGenerator/inc/CosmicCORSIKA.hh"
+
+ namespace mu2e {
+
+ class CorsikaEventGenerator : public art::EDProducer {
I wrote the Source module, however it wasn't as straightforward as I thought: the source module could access the geometry only after the first event had been processed, so I couldn't extrapolate the cosmic rays to the world borders. In order to avoid ugly solutions (such as storing a first empty event) I had to write also a producer which takes as input the output of the Source module and does the backwards extrapolation. I compared the outputs before and after these changes and they are the same.
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
Collaborator
|
Works for me.
Dave
On Mon, Oct 28, 2019 at 20:23 Rob Kutschke ***@***.***> wrote:
Hi Roberto,
Adding Andrei and Ray to this.
What do people think about this solution? Is this an example of good
separation of function - it keeps
the source module clean? Or is it an artificial limitation of our geometry
system being out of date and therefore
causing artificial module pollution. I can see arguments on both sides but
I think it's the latter.
In our present design:
- you get *nominal* geometry from the geeometry service; it is fixed for a
full art run.
- you get aligned geometry from the ProditionsService
- many geometry elements will only be in the GeometryService
- The dimensions of the world volume will only be in the GeometryService;
they are not run dependent
but they may be production campaign dependent.
I believe that it's time to move the initialization of the GeometryService
to BeginJob. We would leave
ProditionsService as it is - it initializes objects on demand and requires
a SubRunID in order to do its job.
In this way code that needs only *nominal* Geometry can run anytime after
BeginJob. This would allow
Roberto to return to a single module solution.
Any comments on this? Do we need to broaden the discussion?
Rob
> On Oct 28, 2019, at 6:41 PM, Stefano Roberto Soleti <
***@***.***> wrote:
>
> @soleti commented on this pull request.
>
> In EventGenerator/src/CORSIKAEventGenerator_module.cc:
>
> > +#include "art/Framework/Principal/Handle.h"
> +#include "fhiclcpp/ParameterSet.h"
> +#include "messagefacility/MessageLogger/MessageLogger.h"
> +
> +// C++ includes.
> +#include <iostream>
> +#include <sstream>
> +#include <string>
> +#include <vector>
> +#include <memory>
> +
> +#include "EventGenerator/inc/CosmicCORSIKA.hh"
> +
> + namespace mu2e {
> +
> + class CorsikaEventGenerator : public art::EDProducer {
>
> I wrote the Source module, however it wasn't as straightforward as I
thought: the source module could access the geometry only after the first
event had been processed, so I couldn't extrapolate the cosmic rays to the
world borders. In order to avoid ugly solutions (such as storing a first
empty event) I had to write also a producer which takes as input the output
of the Source module and does the backwards extrapolation. I compared the
outputs before and after these changes and they are the same.
>
> —
> You are receiving this because you were assigned.
> Reply to this email directly, view it on GitHub, or unsubscribe.
>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#35?email_source=notifications&email_token=ABAH576JRJB6RGH2EEH7LQLQQ6UAXA5CNFSM4JDUXDQ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOECPDJ5Y#issuecomment-547239159>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABAH574BPCJYXSYAXX2DSHLQQ6UAXANCNFSM4JDUXDQQ>
.
--
David Nathan Brown Dave_Brown@lbl.gov
Office Phone (510) 486-7261 Fax 495-2957
Lawrence Berkeley National Lab
MS 50R5008 (50-6026C) Berkeley, CA 94720
|
Contributor
|
Hi Rob,
I believe that it's time to move the initialization of the GeometryService to BeginJob.
Yes, that's what I was pushing for for years. I agree with your
summary. The information coming from GeometryService is fixed for the
duration of a job, there is no reason to hide it at begin job. It
does create inconvenience and requires ugly workarounds in more than
one place.
Andrei
…On Tue, 29 Oct 2019, Robert K Kutschke wrote:
Hi Roberto,
Adding Andrei and Ray to this.
What do people think about this solution? Is this an example of good separation of function - it keeps
the source module clean? Or is it an artificial limitation of our geometry system being out of date and therefore
causing artificial module pollution. I can see arguments on both sides but I think it's the latter.
In our present design:
- you get *nominal* geometry from the geeometry service; it is fixed for a full art run.
- you get aligned geometry from the ProditionsService
- many geometry elements will only be in the GeometryService
- The dimensions of the world volume will only be in the GeometryService; they are not run dependent
but they may be production campaign dependent.
I believe that it's time to move the initialization of the GeometryService to BeginJob.
We would leave
ProditionsService as it is - it initializes objects on demand and requires a SubRunID in order to do its job.
In this way code that needs only *nominal* Geometry can run anytime after BeginJob. This would allow
Roberto to return to a single module solution.
Any comments on this? Do we need to broaden the discussion?
Rob
> On Oct 28, 2019, at 6:41 PM, Stefano Roberto Soleti ***@***.***> wrote:
>
> @soleti commented on this pull request.
>
> In EventGenerator/src/CORSIKAEventGenerator_module.cc:
>
> > +#include "art/Framework/Principal/Handle.h"
> +#include "fhiclcpp/ParameterSet.h"
> +#include "messagefacility/MessageLogger/MessageLogger.h"
> +
> +// C++ includes.
> +#include <iostream>
> +#include <sstream>
> +#include <string>
> +#include <vector>
> +#include <memory>
> +
> +#include "EventGenerator/inc/CosmicCORSIKA.hh"
> +
> + namespace mu2e {
> +
> + class CorsikaEventGenerator : public art::EDProducer {
>
> I wrote the Source module, however it wasn't as straightforward as I thought: the source module could access the geometry only after the first event had been processed, so I couldn't extrapolate the cosmic rays to the world borders. In order to avoid ugly solutions (such as storing a first empty event) I had to write also a producer which takes as input the output of the Source module and does the backwards extrapolation. I compared the outputs before and after these changes and they are the same.
>
> —
> You are receiving this because you were assigned.
> Reply to this email directly, view it on GitHub, or unsubscribe.
>
|
Collaborator
|
We need to disentangle this discussion from Roberto's pull request. If his code does what it's supposed to and isn't wrong we should approve it. We can then fix the Geometry initialization, and after that request updates from dependent code with workarounds. |
Contributor
|
Hi Dave,
This refers to the discussion to initiailize geometry service at an earlier time so that he can use it
in his Source module - right?
If so, I agree.
Rob
… On Oct 29, 2019, at 10:39 AM, David Brown ***@***.***> wrote:
We need to disentangle this discussion from Roberto's pull request. If his code does what it's supposed to and isn't wrong we should approve it. We can then fix the Geometry initialization, and after that request updates from dependent code with workarounds.
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
Contributor
|
Hi Everyone,
We have 3 votes in favour - Dave, Andrei and me.
Ray?
In a separate discussion Andrei requested that we ensure that the GeometryService information
is available at module c'tor time. This means moving the initialization of the geometry earlier
than in my original proposal. We can do it in the service c'tor. That works for me.
There is a second option, to register callback for sPreModuleConstruction but that feels less
transparent without an offsetting benefit.
Rob
… On Oct 29, 2019, at 10:33 AM, Andrei Gaponenko ***@***.***> wrote:
Hi Rob,
> I believe that it's time to move the initialization of the GeometryService to BeginJob.
Yes, that's what I was pushing for for years. I agree with your
summary. The information coming from GeometryService is fixed for the
duration of a job, there is no reason to hide it at begin job. It
does create inconvenience and requires ugly workarounds in more than
one place.
Andrei
On Tue, 29 Oct 2019, Robert K Kutschke wrote:
> Hi Roberto,
>
> Adding Andrei and Ray to this.
>
> What do people think about this solution? Is this an example of good separation of function - it keeps
> the source module clean? Or is it an artificial limitation of our geometry system being out of date and therefore
> causing artificial module pollution. I can see arguments on both sides but I think it's the latter.
>
> In our present design:
> - you get *nominal* geometry from the geeometry service; it is fixed for a full art run.
> - you get aligned geometry from the ProditionsService
> - many geometry elements will only be in the GeometryService
> - The dimensions of the world volume will only be in the GeometryService; they are not run dependent
> but they may be production campaign dependent.
>
> I believe that it's time to move the initialization of the GeometryService to BeginJob.
> We would leave
> ProditionsService as it is - it initializes objects on demand and requires a SubRunID in order to do its job.
>
> In this way code that needs only *nominal* Geometry can run anytime after BeginJob. This would allow
> Roberto to return to a single module solution.
>
> Any comments on this? Do we need to broaden the discussion?
>
> Rob
>
>
>
>
>> On Oct 28, 2019, at 6:41 PM, Stefano Roberto Soleti ***@***.***> wrote:
>>
>> @soleti commented on this pull request.
>>
>> In EventGenerator/src/CORSIKAEventGenerator_module.cc:
>>
>>> +#include "art/Framework/Principal/Handle.h"
>> +#include "fhiclcpp/ParameterSet.h"
>> +#include "messagefacility/MessageLogger/MessageLogger.h"
>> +
>> +// C++ includes.
>> +#include <iostream>
>> +#include <sstream>
>> +#include <string>
>> +#include <vector>
>> +#include <memory>
>> +
>> +#include "EventGenerator/inc/CosmicCORSIKA.hh"
>> +
>> + namespace mu2e {
>> +
>> + class CorsikaEventGenerator : public art::EDProducer {
>>
>> I wrote the Source module, however it wasn't as straightforward as I thought: the source module could access the geometry only after the first event had been processed, so I couldn't extrapolate the cosmic rays to the world borders. In order to avoid ugly solutions (such as storing a first empty event) I had to write also a producer which takes as input the output of the Source module and does the backwards extrapolation. I compared the outputs before and after these changes and they are the same.
>>
>> —
>> You are receiving this because you were assigned.
>> Reply to this email directly, view it on GitHub, or unsubscribe.
>>
>
|
Contributor
Author
|
The latest commit includes both the producer and the source module and it gives the same results of the previous version. My opinion is that it should be merged into master after this review is over and then I will do another pull request once the initialization of the Geometry service gets changed. |
Collaborator
|
I agree. I sign off on this pull request.
David Brown
… On Oct 30, 2019, at 17:53, Stefano Roberto Soleti ***@***.***> wrote:
The latest commit includes both the producer and the source module and it gives the same results of the previous version. My opinion is that it should be merged into master after this review is over and then I will do another pull request once the initialization of the Geometry service gets changed.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
sdifalco
approved these changes
Oct 31, 2019
sdifalco
left a comment
sdifalco
left a comment
Collaborator
There was a problem hiding this comment.
I think Rob has already pointed out the most important issues with the interface with art and the rest of the Offline. I consider all the submitted changes worth to be submitted and valuable. I should try to run Corsika to find more potential issues. For the moment I approve the pull request
Contributor
|
Here is where I think we stand:
So once 1) and 2) are off the table I will be ready to approve. |
rlcee
reviewed
Oct 31, 2019
rlcee
left a comment
rlcee
left a comment
Collaborator
There was a problem hiding this comment.
After twice clicking on lines of code, entering comment text, and selecting "start a review".
None has seen an email alert or even my comment on github. Now I am filling in
a new box that appeared with a button "submit review".
Also I see my comments are labeled "outdated" probably because the code moved to
a new subdirectory.
| const HepLorentzVector mom4 = particle.momentum(); | ||
|
|
||
| const Hep3Vector position( | ||
| wrapvar(particle.position().x() + _flat(), _targetBoxXmin - _showerAreaExtension, _targetBoxXmax + _showerAreaExtension) + _cosmicReferencePointInMu2e.x(), |
Collaborator
There was a problem hiding this comment.
Sorry if I am not following the logic. If your accepted range is, for example, 0-10, then particles hitting at 15 get wrapped into 0-10, and so do particles that hit at 25. So it seems that you are potentially compressing the shower. Calling flat on each particle instead of once per event will scramble the shower correlations, won't it?
Contributor
Author
There was a problem hiding this comment.
This is an interesting observation and the logic is explained here: https://internal.dunescience.org/doxygen/classevgen_1_1CORSIKAGen.html#details. I think it makes sense but please let me know if you find a flaw in it.
Collaborator
There was a problem hiding this comment.
The link you point to says that if a shower has particles outside
the detector box, then those are folded into the
detector box, and then treated as a separate event coming at
a separate random time. I don't see that second part -
a separate event at another time. I think there are other ways to
handle it, but the issue of showers larger that the detector box
should be documented in plain English.
It looks like your last talk doesn't discuss it. I'm OK with
finishing this code review with a note that you should still
present a plan on this issue.
Contributor
|
it looks that I'm the last one left - signing off |
pavel1murat
approved these changes
Nov 1, 2019
Contributor
|
btw, why the Corsica module goes to Analyses and not EventGenerators? |
Contributor
Author
|
Hi Pasha, there is a Source module that reads the CORSIKA binaries, a Producer that creates the data products (in EventGenerators) and an analyzer that creates a TTree to check the output.
… On Oct 31, 2019, at 21:55, Pavel Murat ***@***.***> wrote:
btw, why the Corsica module goes to Analyses and not EventGenerators?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#35?email_source=notifications&email_token=ABR3QVKJ2KSOEDTQDW6OA6DQROZE3A5CNFSM4JDUXDQ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOECZ7W4I#issuecomment-548666225>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABR3QVOP2L7Y47PWNZVID3TQROZE3ANCNFSM4JDUXDQQ>.
|
Contributor
|
Hi Roberto,
On Fri, 1 Nov 2019, Ray Culbertson wrote:
The link you point to says that if a shower has particles outside
the detector box, then those are folded into the
detector box, and then treated as a separate event coming at
a separate random time. I don't see that second part -
a separate event at another time. I think there are other ways to
handle it, but the issue of showers larger that the detector box
should be documented in plain English.
To take it to the extreme, think of simulating a 1 km^2 air shower, then
folding it all into a 1 m^2 detector box using the current method. The
result will clearly be unphysical. If we do not fudge the time, which
has its own difficulties, it is OK to shift the shower as a whole once,
but after that particles outside the detector box should be discarded.
Andrei
… It looks like your last talk doesn't discuss it. I'm OK with
finishing this code review with a note that you should still
present a plan on this issue.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_Mu2e_Offline_pull_35-23discussion-5Fr341591570&d=DwICaQ&c=gRgGjJ3BkIsb5y6s49QqsA&r=O47fc5vzDTR2V_gla4Ub0Q&m=jU4BRqcq2acx6VEmXDLbQ4clzN-IGvkH0jRjrw7bHfI&s=u1I4oFrOGqBgV798hKSDYZijd-iMCkf6V9pMvONU3Vg&e=
|
Contributor
Author
|
It’s not unphysical if you take into account the fact that you are considering a smaller area in the flux normalization (when you calculate the corresponding live time) and if, you’re right, we assign a random time to the re-packaged particles. The re-packaged particles don’t need to belong to a different event though (in the art sense). However, my understanding was that the time was managed at a later stage, so it didn’t matter here. If you have a better way to manage this I am happy to hear it and implement it. We could also discuss this in a meeting.
Roberto
… On Nov 1, 2019, at 09:17, gaponenko ***@***.***> wrote:
Hi Roberto,
On Fri, 1 Nov 2019, Ray Culbertson wrote:
> The link you point to says that if a shower has particles outside
> the detector box, then those are folded into the
> detector box, and then treated as a separate event coming at
> a separate random time. I don't see that second part -
> a separate event at another time. I think there are other ways to
> handle it, but the issue of showers larger that the detector box
> should be documented in plain English.
To take it to the extreme, think of simulating a 1 km^2 air shower, then
folding it all into a 1 m^2 detector box using the current method. The
result will clearly be unphysical. If we do not fudge the time, which
has its own difficulties, it is OK to shift the shower as a whole once,
but after that particles outside the detector box should be discarded.
Andrei
> It looks like your last talk doesn't discuss it. I'm OK with
> finishing this code review with a note that you should still
> present a plan on this issue.
>
> --
> You are receiving this because you are subscribed to this thread.
> Reply to this email directly or view it on GitHub:
> https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_Mu2e_Offline_pull_35-23discussion-5Fr341591570&d=DwICaQ&c=gRgGjJ3BkIsb5y6s49QqsA&r=O47fc5vzDTR2V_gla4Ub0Q&m=jU4BRqcq2acx6VEmXDLbQ4clzN-IGvkH0jRjrw7bHfI&s=u1I4oFrOGqBgV798hKSDYZijd-iMCkf6V9pMvONU3Vg&e=
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#35?email_source=notifications&email_token=ABR3QVPW7WW3OY5ICIRWLITQRRJBRA5CNFSM4JDUXDQ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEC3MOHA#issuecomment-548849436>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABR3QVK5ITJ2AU2GIUV5H2DQRRJBRANCNFSM4JDUXDQQ>.
|
Contributor
|
Hi Roberto,
On Fri, 1 Nov 2019, Stefano Roberto Soleti wrote:
It’s not unphysical if you take into account the fact that you are
considering a smaller area in the flux normalization (when you
calculate the corresponding live time) and if, you’re right, we
assign a random time to the re-packaged particles. The re-packaged
particles don’t need to belong to a different event though (in the
art sense). However, my understanding was that the time was managed
at a later stage, so it didn’t matter here.
The the time offsets are per primary. So everything in one original
shower will be shifted by the same amount, regardless of whether the
particle hit the box or was "folded" into it.
By unphysical I mean the density of particles in that simulated event
will be million times more than in the actual shower. If you do not
care about correlations between particles and only about the
integrated flux it may be compensated by the live time, but the
purpose of dealing with air shower simulations in the first place is
to get the correlations right. I think the right way is to drop
particles outside the box instead of folding them into the same event.
If you have a better way
to manage this I am happy to hear it and implement it. We could also
discuss this in a meeting.
We have two shapes: the "detector box", and the shower outline.
Depending on the event the shower may be smaller or larger than the
detector box. Instead of trying to fit the shower in the box in a
single event, we need to randomly draw a uniform 2D offset vector
between the two shapes, and take just the portion of the shower that
overlaps the detector. When the same shower shows up again in the
resampling we'll randomly get another part of it in the detector, in a
different event.
Regards,
Andrei
…
Roberto
> On Nov 1, 2019, at 09:17, gaponenko ***@***.***> wrote:
>
> Hi Roberto,
>
> On Fri, 1 Nov 2019, Ray Culbertson wrote:
>
> > The link you point to says that if a shower has particles outside
> > the detector box, then those are folded into the
> > detector box, and then treated as a separate event coming at
> > a separate random time. I don't see that second part -
> > a separate event at another time. I think there are other ways to
> > handle it, but the issue of showers larger that the detector box
> > should be documented in plain English.
>
> To take it to the extreme, think of simulating a 1 km^2 air shower, then
> folding it all into a 1 m^2 detector box using the current method. The
> result will clearly be unphysical. If we do not fudge the time, which
> has its own difficulties, it is OK to shift the shower as a whole once,
> but after that particles outside the detector box should be discarded.
>
> Andrei
>
> > It looks like your last talk doesn't discuss it. I'm OK with
> > finishing this code review with a note that you should still
> > present a plan on this issue.
> >
> > --
> > You are receiving this because you are subscribed to this thread.
> > Reply to this email directly or view it on GitHub:
> > https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_Mu2e_Offline_pull_35-23discussion-5Fr341591570&d=DwICaQ&c=gRgGjJ3BkIsb5y6s49QqsA&r=O47fc5vzDTR2V_gla4Ub0Q&m=jU4BRqcq2acx6VEmXDLbQ4clzN-IGvkH0jRjrw7bHfI&s=u1I4oFrOGqBgV798hKSDYZijd-iMCkf6V9pMvONU3Vg&e=
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub <https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_Mu2e_Offline_pull_35-3Femail-5Fsource-3Dnotifications-26email-5Ftoken-3DABR3QVPW7WW3OY5ICIRWLITQRRJBRA5CNFSM4JDUXDQ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEC3MOHA-23issuecomment-2D548849436&d=DwIFaQ&c=gRgGjJ3BkIsb5y6s49QqsA&r=O47fc5vzDTR2V_gla4Ub0Q&m=LXc-pBhNpUXE_Vj87o79GNCcBlGTGCqRpZQADLz9iVs&s=9dzlrOltoOMIqzMU0t7Qzn4dFx4OP9eDpY3Za_PWiEc&e= >, or unsubscribe <https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_notifications_unsubscribe-2Dauth_ABR3QVK5ITJ2AU2GIUV5H2DQRRJBRANCNFSM4JDUXDQQ&d=DwIFaQ&c=gRgGjJ3BkIsb5y6s49QqsA&r=O47fc5vzDTR2V_gla4Ub0Q&m=LXc-pBhNpUXE_Vj87o79GNCcBlGTGCqRpZQADLz9iVs&s=AA6ZGd1pVUa_-9Ei16Yjp0yVPjva_uA8CKqXr2Z7A0o&e= >.
>
--
You are receiving this because you commented.
Reply to this email directly or view it on GitHub:
https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_Mu2e_Offline_pull_35-23issuecomment-2D548874822&d=DwIFaQ&c=gRgGjJ3BkIsb5y6s49QqsA&r=O47fc5vzDTR2V_gla4Ub0Q&m=LXc-pBhNpUXE_Vj87o79GNCcBlGTGCqRpZQADLz9iVs&s=uTqLXQAJBso6B8E5qowxbkgIe0b0KH7VFaUwg8_oLSw&e=
|
Contributor
Author
|
Hi Andrei
On Nov 1, 2019, at 10:53, gaponenko ***@***.***> wrote:
Hi Roberto,
On Fri, 1 Nov 2019, Stefano Roberto Soleti wrote:
> It’s not unphysical if you take into account the fact that you are
> considering a smaller area in the flux normalization (when you
> calculate the corresponding live time) and if, you’re right, we
> assign a random time to the re-packaged particles. The re-packaged
> particles don’t need to belong to a different event though (in the
> art sense). However, my understanding was that the time was managed
> at a later stage, so it didn’t matter here.
The the time offsets are per primary. So everything in one original
shower will be shifted by the same amount, regardless of whether the
particle hit the box or was "folded" into it.
By unphysical I mean the density of particles in that simulated event
will be million times more than in the actual shower. If you do not
care about correlations between particles and only about the
integrated flux it may be compensated by the live time, but the
purpose of dealing with air shower simulations in the first place is
to get the correlations right. I think the right way is to drop
particles outside the box instead of folding them into the same event.
Yes, you’re right. I didn’t see any effect in the plots I made because the integrated flux is the same.
> If you have a better way
> to manage this I am happy to hear it and implement it. We could also
> discuss this in a meeting.
We have two shapes: the "detector box", and the shower outline.
Depending on the event the shower may be smaller or larger than the
detector box. Instead of trying to fit the shower in the box in a
single event, we need to randomly draw a uniform 2D offset vector
between the two shapes, and take just the portion of the shower that
overlaps the detector. When the same shower shows up again in the
resampling we'll randomly get another part of it in the detector, in a
different event.
I fixed this by “tiling” the shower area and creating a new event for each tile that contains at least a particle. I get the same number of showers per event as CRY and the integrated flux is the same as before. I am happy to show the entire chain at the following simulation meeting.
Best,
Roberto
…
Regards,
Andrei
>
> Roberto
>
>
> > On Nov 1, 2019, at 09:17, gaponenko ***@***.***> wrote:
> >
> > Hi Roberto,
> >
> > On Fri, 1 Nov 2019, Ray Culbertson wrote:
> >
> > > The link you point to says that if a shower has particles outside
> > > the detector box, then those are folded into the
> > > detector box, and then treated as a separate event coming at
> > > a separate random time. I don't see that second part -
> > > a separate event at another time. I think there are other ways to
> > > handle it, but the issue of showers larger that the detector box
> > > should be documented in plain English.
> >
> > To take it to the extreme, think of simulating a 1 km^2 air shower, then
> > folding it all into a 1 m^2 detector box using the current method. The
> > result will clearly be unphysical. If we do not fudge the time, which
> > has its own difficulties, it is OK to shift the shower as a whole once,
> > but after that particles outside the detector box should be discarded.
> >
> > Andrei
> >
> > > It looks like your last talk doesn't discuss it. I'm OK with
> > > finishing this code review with a note that you should still
> > > present a plan on this issue.
> > >
> > > --
> > > You are receiving this because you are subscribed to this thread.
> > > Reply to this email directly or view it on GitHub:
> > > https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_Mu2e_Offline_pull_35-23discussion-5Fr341591570&d=DwICaQ&c=gRgGjJ3BkIsb5y6s49QqsA&r=O47fc5vzDTR2V_gla4Ub0Q&m=jU4BRqcq2acx6VEmXDLbQ4clzN-IGvkH0jRjrw7bHfI&s=u1I4oFrOGqBgV798hKSDYZijd-iMCkf6V9pMvONU3Vg&e=
> > —
> > You are receiving this because you were mentioned.
> > Reply to this email directly, view it on GitHub <https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_Mu2e_Offline_pull_35-3Femail-5Fsource-3Dnotifications-26email-5Ftoken-3DABR3QVPW7WW3OY5ICIRWLITQRRJBRA5CNFSM4JDUXDQ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEC3MOHA-23issuecomment-2D548849436&d=DwIFaQ&c=gRgGjJ3BkIsb5y6s49QqsA&r=O47fc5vzDTR2V_gla4Ub0Q&m=LXc-pBhNpUXE_Vj87o79GNCcBlGTGCqRpZQADLz9iVs&s=9dzlrOltoOMIqzMU0t7Qzn4dFx4OP9eDpY3Za_PWiEc&e= >, or unsubscribe <https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_notifications_unsubscribe-2Dauth_ABR3QVK5ITJ2AU2GIUV5H2DQRRJBRANCNFSM4JDUXDQQ&d=DwIFaQ&c=gRgGjJ3BkIsb5y6s49QqsA&r=O47fc5vzDTR2V_gla4Ub0Q&m=LXc-pBhNpUXE_Vj87o79GNCcBlGTGCqRpZQADLz9iVs&s=AA6ZGd1pVUa_-9Ei16Yjp0yVPjva_uA8CKqXr2Z7A0o&e= >.
> >
>
>
>
> --
> You are receiving this because you commented.
> Reply to this email directly or view it on GitHub:
> https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_Mu2e_Offline_pull_35-23issuecomment-2D548874822&d=DwIFaQ&c=gRgGjJ3BkIsb5y6s49QqsA&r=O47fc5vzDTR2V_gla4Ub0Q&m=LXc-pBhNpUXE_Vj87o79GNCcBlGTGCqRpZQADLz9iVs&s=uTqLXQAJBso6B8E5qowxbkgIe0b0KH7VFaUwg8_oLSw&e=
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#35?email_source=notifications&email_token=ABR3QVIYQZWJHPQIG5BC6ULQRRUKFA5CNFSM4JDUXDQ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEC3VYAI#issuecomment-548887553>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABR3QVLAFLFVNWXG44EDYM3QRRUKFANCNFSM4JDUXDQQ>.
|
Contributor
|
Two things.
There have been a lot of changes since you first comm |
Contributor
Author
|
Hi Rob,
Basically, instead of folding back every particle inside the target box in the same event I create N different events, where N is the number of tiles with at least one particle. The size of the tile is the size of the target box. However, I agree I should give a presentation with all the changes. Wednesday sounds good.
I did a clean clone and it runs fine.
Best,
Roberto
… On Nov 4, 2019, at 15:04, Rob Kutschke ***@***.***> wrote:
Two things.
I don't understand your tiling solution as well as I think I should. Can you give a presentation on that at the meeting this Wednesday?
You have responded to a lot of feedback and made a lot of changes. Have you done a clean clone and rebuild and run a test job?
There have been a lot of changes since you first comm
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#35?email_source=notifications&email_token=ABR3QVO2VR4NWUIAARNCNNLQSCS7NA5CNFSM4JDUXDQ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEDBAYGY#issuecomment-549587995>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABR3QVM7APH6T6JZDEGKIDLQSCS7NANCNFSM4JDUXDQQ>.
|
Contributor
|
Following yesterdays's talk I propose to merge this pull request today. Unless I hear otherwise I will |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request includes: