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

Skeletton for case studies analyses packages #199

Merged
merged 28 commits into from Oct 5, 2022

Conversation

forthommel
Copy link
Contributor

@forthommel forthommel commented Aug 9, 2022

A skeletton to ease the definition of standalone analyses packages (e.g. for specific case studies) is introduced.
The building principle is as follows:

  • the analyses folder is scanned recursively
  • for each folder_name subdirectory, if a "package-like" directory structure is found (i.e. a src/ and a include/ subdirectory are present), a new libFCCAnalysis_[folder_name].so shared object is built, with associated ROOT dictionary files (built using the LCG reflexion tool)
  • if not, the scan proceeds to the eventual folder_name subdirectories.

Checklist of features to be implemented in this scope:

  • script to generate automatically a new package skeletton given an user-defined analysis/namespace name, with correct directory structure and corresponding classes.h (namespace exposition) and classes_def.xml (LCG dictionary generation) files
  • handle unit tests/CI workflows
  • optional compilation of one or several analyses packages Postponed to a follow-up PR

Reference to this development

@forthommel forthommel marked this pull request as draft August 9, 2022 14:38
@clementhelsens
Copy link
Contributor

Thanks @forthommel , this is a nice first prototype, I think we should discuss this in our weekly coordination meeting. Would you be able to join us on Friday morning at 9h30? (@gganis , @vvolkl , @kjvbrt )

@vvolkl
Copy link
Member

vvolkl commented Aug 10, 2022

Very nice indeed, I just have some comments on the implementation, will post these here when I went through the whole PR.

@forthommel
Copy link
Contributor Author

forthommel commented Aug 11, 2022

Strangely I cannot reproduce the analysis_example test failure on my work bench, neither through ctest:
100% tests passed, 0 tests failed out of 33

nor running manually python config/FCCAnalysisRun.py analyses/analysis_example/scripts/analysis_example.py --test --nevents 100 --bench... Investigating!

@clementhelsens
Copy link
Contributor

looks like analysis_example is unknown to ROOT
https://github.com/HEP-FCC/FCCAnalyses/runs/7784922633?check_suite_focus=true#step:8:40

@forthommel
Copy link
Contributor Author

looks like analysis_example is unknown to ROOT https://github.com/HEP-FCC/FCCAnalyses/runs/7784922633?check_suite_focus=true#step:8:40

Indeed, although this does not get spotted in my environment...

@vvolkl
Copy link
Member

vvolkl commented Aug 11, 2022

My suggestion would be to move the templates out of config/analysis_builder.py and into separate files. For CMake templates the convention is to use the final filename suffixed with .in, which has the advantage that it will come up readily when looking where a generated file comes from.

For similar reasons I'm a bit sceptical about scanning directories, as this may be "surprising" for a future maintainer, but I'd also merge that as is.

@clementhelsens
Copy link
Contributor

I agree @vvolkl , but I think @forthommel missing item in the original post of this PR is precisely about changing this automatic scan to optional compilation. I think this is something we could discuss tomorrow morning in our meeting as it's more conceptual decision wrt PP rather than a technical thing.

@forthommel forthommel marked this pull request as ready for review August 15, 2022 01:10
@forthommel
Copy link
Contributor Author

@clementhelsens, @vvolkl, I believe the current snapshot is ready for your review. At least if we maintain the intermediate plan discussed last Friday.

@clementhelsens
Copy link
Contributor

thanks @forthommel. I'm not supposed to work today, will review later this week. A first obvious addition I would suggest is to add a dedicated README inside case-studiesand a subsection experimental in the main README pointing to it.

@forthommel
Copy link
Contributor Author

Thanks for the feedback, and sorry for spamming you on your holidays @clementhelsens :)
Sure, I just pushed a (brief, can surely be greatly improved) README to the directory, directly linked at by the main README.

@forthommel
Copy link
Contributor Author

Hi @clementhelsens, @vvolkl ! I don't know if you had the time/opportunity to have a look at this review lately?

Copy link
Contributor

@clementhelsens clementhelsens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@forthommel , thanks, merging for the tutorial

@clementhelsens clementhelsens merged commit cf38f36 into HEP-FCC:master Oct 5, 2022
@forthommel forthommel deleted the case-studies branch October 5, 2022 09:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants