alcap-org / AlcapDAQ

Alcap DAQ
alcap-org.github.io
8 stars 0 forks source link

Feature/ae al50 analysis #260

Closed AndrewEdmonds11 closed 9 years ago

AndrewEdmonds11 commented 9 years ago

So I'm opening this pull request before I attempt to rebase since that's causing me a load of problems.

Most of my work is in its own subfolders but the things I have changed in common areas and probably need discussion are:

There are some modules (namely the PlotTAP_Energy and TMEViewer) which I think could be moved into the main source folders.

I won't merge this in for the time being until I get people's thoughts...

benkrikler commented 9 years ago

I added a PlotTAPEnergy module for my work, and I'm guessing we did the same thing and copied the code from the other PlotTAP... modules, since they look almost the same. If you agree that they do the same thing, then we should just remove one of them.

We've also both added a module that makes the EvdE plots from TMEs. Mine has ActiveSi in the name, but perhaps we can make yours clearer as well? Especially in the Include guard we should be careful (maybe stick Al50b or Andy in it) whilst it sits in the Al50b directory since a module with the same name in the src directories might be mis-leading and clash with this one.

Definitely I would undo the default parameter changes to the PCF and we should look at providing a better mechanism to supply it with different PCF values or to set them all to 0.

The overflow check is nice but it would also be nice to have that run-time configurable.

I'm a bit unsure about the change to MakeDetectorPulses. I have a feeling we spoke about it but I think I'd fixed that issue already. My memory might be failing though and I'm also a bit jet-lagged from Japan still so I'm not really sure what's going on right now to be honest...

jrquirk commented 9 years ago

Since you've made no substantial changes to common code, apart from what Ben mentioned above, no one should take issue with this I think.

As for the germanium code, the bulk of the changes that aren't compatible between us is that I made some of the parameters configurable. That is an issue we'll deal with when I merge back. This looks good.

AndrewEdmonds11 commented 9 years ago

I added a PlotTAPEnergy module for my work, and I'm guessing we did the same thing and copied the code from the other PlotTAP... modules, since they look almost the same. If you agree that they do the same thing, then we should just remove one of them.

Agreed. Deleted mine

We've also both added a module that makes the EvdE plots from TMEs. Mine has ActiveSi in the name, but perhaps we can make yours clearer as well?

Agreed. Renamed mine to TME_Al50_EvdE. It also somewhat indicates that this isn't necessarily ready to be made permanent

Definitely I would undo the default parameter changes to the PCF and we should look at providing a better mechanism to supply it with different PCF values or to set them all to 0.

Done. I'm sure there's a clever way to do this in git but I just copied them from the commits where I changed them.

The overflow check is nice but it would also be nice to have that run-time configurable.

Just commented out for the time being. I remember having problems getting configure options down into the PCF since it is not a module itself. I think we need to revisit where this goes - I propose something similar to what I'm going to be doing with the scattered muons in the TMEs but I guess we should see how that pans out...

That is an issue we'll deal with when I merge back

Cool. I'll leave it to you then.

Right, I'll merge back in now. Hopefully, it won't all get screwed up...