search

JeffersonLab / iguana

Preservation of common physics data analysis algorithms. Currently focused on HIPO data.
https://jeffersonlab.github.io/iguana/
GNU Lesser General Public License v3.0
2 stars 6 forks source link

Filter is private in FiducialFilter #309

Closed dglazier closed 1 day ago

dglazier commented 1 week ago

Action functions should be available for users. Can we make FiducialFilter::Filter public ?

dglazier commented 1 week ago

Same for PhotonGBTFilter

dglazier commented 1 week ago

Also PhotonGBTFilter::GetCaloMap would be useful as public

c-dilks commented 1 week ago

These methods are private since they are not actually action functions; we want to be able to change their parameters without any downstream impact, for when we add "vector" action functions (e.g. https://github.com/JeffersonLab/iguana/issues/227).

We could make these methods public for now, and provide a warning in the documentation that we are free to change or remove them in any future release; or we could wait until we actually have vector action functions for those algorithms.

Let me know what you prefer; if you were to use these functions in clas12root, you will likely end up with a version compatibility mess.

I'm not sure when I'll have time to make vector action functions, but contributions are always welcome.

dglazier commented 6 days ago

In clas12root the user works with region_particle pointers, which contain pointers to the relevent banks. The user is supplied with a vector of region_particles in the event loop to do their analysis. In the clas12root iguana Filter interface the vector of particles is looped over and if a particle fails the filter it is removed from the vector. After filtering the user will continue their analysis as before. For example FiducialFilter, Has a return bool function which returns the iguana "action" function Filter result.

https://github.com/dglazier/clas12root/blob/iguana_filter/iguana/Filters.h#L76

And a functions which performs the loop over all particles via a utility function,

https://github.com/dglazier/clas12root/blob/iguana_filter/iguana/Filters.h#L95

After calling this latter function in the event loop they will have access to the filtered particles only. For this to be able to work clas12root needs to be able to access the currently private data members. Note also includes iguana::clas12::PhotonGBTFilter::calo_row_data in addition to the previous members. Sidenote : having PhotonGBTFilter::GetCaloMap return a vector of pairs rather than a map would likely be better for performance, as maps are bad for the cache.

dglazier commented 2 days ago

In addition we now have private functions which must be called prior to the action. e.g. ZVertexFilter::Reload This looks like it might be better as an public Algorithm method, rather than specified for each algorithm. This would allow looping over collections of algorithms. Will the multithreading changes effect being able to run single thread. Currently I cannot switch the clas12root interface to iguana current version.

c-dilks commented 1 day ago

Closing, since this is a duplicate of #227. I prefer we make proper action functions; a few other algorithms need them too.

In addition we now have private functions which must be called prior to the action. e.g. ZVertexFilter::Reload

Call ZVertexFilter::PrepareEvent, not Reload; see the documentation: