Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Filter is private in FiducialFilter #309

Closed
dglazier opened this issue Nov 20, 2024 · 6 comments
Closed

Filter is private in FiducialFilter #309

dglazier opened this issue Nov 20, 2024 · 6 comments

Comments

@dglazier
Copy link
Contributor

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

@dglazier
Copy link
Contributor Author

Same for PhotonGBTFilter

@dglazier
Copy link
Contributor Author

Also PhotonGBTFilter::GetCaloMap would be useful as public

@c-dilks
Copy link
Member

c-dilks commented Nov 20, 2024

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. #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
Copy link
Contributor Author

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
Copy link
Contributor Author

dglazier commented Nov 25, 2024

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
Copy link
Member

c-dilks commented Nov 26, 2024

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:

@c-dilks c-dilks closed this as completed Nov 26, 2024
@github-project-automation github-project-automation bot moved this from Unsorted to Done in Iguana Nov 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

No branches or pull requests

2 participants