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

Import Framework Code #82

Merged
merged 11 commits into from
Apr 23, 2020
Merged

Import Framework Code #82

merged 11 commits into from
Apr 23, 2020

Conversation

msmk0
Copy link
Contributor

@msmk0 msmk0 commented Apr 1, 2020

This imports the framework code into the main repository and integrates it into the build system. The code is mostly imported as-is with only minor fixes to adapt to change in the core library. The code has been reorganized in parts to avoid naming collisions/ confusion and now is stored as follows

Examples/
  Algorithm/ # contains some code that was previously in plugins
  Detectors/ # contains also the magnetic fields
  Framework/ # formerly `Core`
  Run/ # formerly `Examples` to clarify that this contains full chain tools
  Io/ # contains HepMC3/Json/Obj code that was previously in plugins

Renaming this module to Examples was done to clarify the intention. I want to avoid that people think this also defines a production-ready event processing framework.

The standalone code can be activated via additional cmake flags

CMAKE_BUILD_EXAMPLES
CMAKE_BUILD_EXAMPLES_DD4HEP
CMAKE_BUILD_EXAMPLES_HEPMC3
...

Any standalone component flag activates the overall standalone flag as well. Dependencies between option flags are handled automatically, e.g. CMAKE_BUILD_EXAMPLES_DD4HEP actives CMAKE_BUILD_DD4HEP_PLUGIN. The standalone TGeo detectors are now always build since the standalone code requires ROOT anyways.

Since there are quite a large number of flags now, I added CMAKE_BUILD_EVERYTHING flag that does exactly what it says. Build artifacts are now placed in <build_dir>/bin to simplify usage.

While the code still uses the ACTFW/ header hierarchy and namespace FW, I updated the CMake target names to match our convention. All internal libraries are prefixed ActsExamples.... Executables are prefixed Acts{Example,Gen,Sim,Rec}... as appropriate.

The two examples that previous existed in Tests/Examples have been moved to Examples/Run/Misc.

Closes #22

@acts-issue-bot acts-issue-bot bot added the Triage label Apr 1, 2020
@msmk0 msmk0 added Component - Core Affects the Core module Component - Examples Affects the Examples module Infrastructure Changes to build tools, continous integration, ... Impact - Major Significant bug and/or affects a lot of modules Feature Development to integrate a new feature and removed Triage labels Apr 1, 2020
@msmk0 msmk0 added this to the 0.21.00 milestone Apr 1, 2020
@msmk0 msmk0 changed the title Standalone WIP Standalone Apr 1, 2020
@paulgessinger
Copy link
Member

FYI: The labels Infrastructure and New Feature are treated mutually exclusive.

@msmk0 msmk0 removed the Infrastructure Changes to build tools, continous integration, ... label Apr 1, 2020
@msmk0
Copy link
Contributor Author

msmk0 commented Apr 1, 2020

FYI: The labels Infrastructure and New Feature are treated mutually exclusive.

Thanks, fixed it.

@paulgessinger
Copy link
Member

Can you rename the options from *_STANDALONE? I think it's pretty opaque what that means / doesn't refer to it being the testing framework.

@msmk0
Copy link
Contributor Author

msmk0 commented Apr 1, 2020

Can you rename the options from *_STANDALONE? I think it's pretty opaque what that means / doesn't refer to it being the testing framework.

I would prefer to leave the naming as Standalone for the reasons discussed in the PR description. To expand a bit on this: the purpose of this code is to run full standalone reconstruction chains both to develop the code, run computation performance studies, and perform physics performance studies.

We could consider them as just another set of tests and place them under the Tests hierarchy. But, the majority of the functionality is not and/or can not be run as automated tests. The code is also quite large and does way more than just test some part of the core code. That is enough reason to organize it as separate from the tests.

The fact that we use our own event processing framework internally is an implementation detail at this point. Thus, the change to Standalone.

@paulgessinger
Copy link
Member

Renaming this module to Standalone was done to clarify the intention. I want to avoid that people think this also defines a production-ready event processing framework.

Actually, I think the name "Standalone" achieves the exact opposite of what you state the purpose is. But ok, I guess we don't have to agree on this.

@HadrienG2
Copy link
Contributor

HadrienG2 commented Apr 1, 2020

If an extra opinion could be helpful in resolving the disagreement, I personally have no idea what "Standalone" means without looking at the documentation, and would prefer another terminology like "full chain", "usage examples" or "integration tests".

(I am well aware that we already have some integration tests in the Acts core, what I'm saying is that ACTSFW is actually the place where 90% of our integration tests reside in practice, and we could well envision to merge the remaining "core" integration tests in there if this terminology was selected.)

@asalzburger
Copy link
Contributor

asalzburger commented Apr 1, 2020

If Standalone is disputed, we could rename it to Examples ?

I would agree with @HadrienG2 here then.

@msmk0 msmk0 changed the title WIP Standalone WIP Import Framework Code Apr 2, 2020
@msmk0
Copy link
Contributor Author

msmk0 commented Apr 2, 2020

I moved the code from Standalone to Examples as discussed. The PR description has been updated accordingly.

I added one more change: executables can now be found in <build_dir>/bin instead of e.g. <build_dir>/Examples/Run/MagnetifField/ActsExampleMagneticField....

P.S. Still need to fix the build.

@asalzburger
Copy link
Contributor

Thank you @msmk0 - I think that's a good compromise.

@asalzburger asalzburger added the 🚧 WIP Work-in-progress label Apr 2, 2020
@msmk0 msmk0 force-pushed the standalone branch 4 times, most recently from 5a25a39 to 3b1e96d Compare April 3, 2020 17:30
@msmk0
Copy link
Contributor Author

msmk0 commented Apr 21, 2020

What's the conclusion on this?

Shall we put that in before the combinatorial Kalman filter or the other way round?

I will add the fixes requested by @Corentin-Allaire and then we can merge. I would suggest to just merge whatever is ready first. But there is no reason why I should go in in any particular order.

@asalzburger
Copy link
Contributor

Ouch, coverage now takes 92m ?

@msmk0 msmk0 force-pushed the standalone branch 3 times, most recently from 364af16 to f3ae0b9 Compare April 22, 2020 15:14
@msmk0
Copy link
Contributor Author

msmk0 commented Apr 22, 2020

@Corentin-Allaire Missing Examples/Scripts have been added. The CMake flags should be used correctly. The geantino recording should work as before.

@Corentin-Allaire
Copy link
Contributor

Just tested and now the Geantino recoding works great !

@asalzburger
Copy link
Contributor

Just tested and now the Geantino recoding works great !

Is that with the ODD or on top of my irk-navigation branch ?

@Corentin-Allaire
Copy link
Contributor

Just tested and now the Geantino recoding works great !

Is that with the ODD or on top of my irk-navigation branch ?

With the ODD, I was testing that the slowness that was caused by the recording of the track step was not here anymore.

@msmk0
Copy link
Contributor Author

msmk0 commented Apr 23, 2020

This could be merged, but the coverage report consistently fails now. Not sure if we want to investigate this first or override it for now?

@asalzburger
Copy link
Contributor

I think we merge this.

Copy link
Contributor

@asalzburger asalzburger left a comment

Choose a reason for hiding this comment

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

There are a few issues we will follow up on this, but for now we merge.

@asalzburger asalzburger merged commit 955e60d into acts-project:master Apr 23, 2020
@msmk0 msmk0 deleted the standalone branch June 26, 2020 10:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component - Core Affects the Core module Component - Examples Affects the Examples module Feature Development to integrate a new feature Impact - Major Significant bug and/or affects a lot of modules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable Response File for job configuration Switch to monorepo
5 participants