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

new ILD models: ILD_l5_v10 and ILD_l5_v11 #288

Merged
merged 13 commits into from
Oct 4, 2023

Conversation

danieljeans
Copy link
Contributor

@danieljeans danieljeans commented Aug 29, 2023

BEGINRELEASENOTES

  • Update to SServices00 to allow individual components to be included/excluded
    -- default behaviour unchanged

  • add new model ILD_l5_v10 : ILD model for ILC
    -- inner silicon (VTX, SIT, FTD) replaced by FCCee_o2_v02-inspired design

  • add new model ILD_l5_v11 : ILD model for FCCee
    -- inner silicon replaced by FCCee_o2_v02-inspired design
    -- beampipe, MDI from FCCee_o2_v02

ENDRELEASENOTES

@danieljeans
Copy link
Contributor Author

danieljeans commented Aug 29, 2023

sorry the SServices00 commit is a mess because of new if() statements enveloping much of the code, so everything is indented...Is there a better way to do it?

@jmcarcell
Copy link
Member

You could not make indentation changes to keep the diff clean and then fix everything with clang-format at the end in the files you are changing

@danieljeans
Copy link
Contributor Author

danieljeans commented Aug 30, 2023

@jmcarcell thanks for the hint. I recommitted without indenting to make the changes easy to see.
Now I'll investigate how to use clang-format : any hints welcome (for next time).
For now I did the next separate commit with only indenting lines.

@danieljeans danieljeans changed the title new ILD models [WIP] new ILD models Sep 1, 2023
@danieljeans danieljeans changed the title [WIP] new ILD models new ILD models: ILD_l5_v10 and ILD_l5_v11 Sep 19, 2023
@danieljeans
Copy link
Contributor Author

danieljeans commented Sep 19, 2023

I think this is now ready for review/merge
edit: that was over-optimistic...I'll try to work out why some tests are failing

@danieljeans
Copy link
Contributor Author

Seems OK now.

Copy link
Contributor

@tmadlener tmadlener left a comment

Choose a reason for hiding this comment

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

There seem to be a few copied modules that are present also in ILD/compact/ILD_common_v02 (at least from a quick look, I might have missed some details that make a copy necessary):

  • `TrackerDiskModuleOut.xml
  • `TrackerDiskModuleIn.xml

these are there for both v10 and v11, and I would prefer using the common definitions if possible, because it is less code to maintain and it is easier to spot differences between the models. We can always make a copy once it becomes necessary.

Additionally, the InnerTrackerBarrelModule{Up,Down} seem to be dupliated between v10, and v11. Can we also put them into the ILD_common_v02 folder and reference them from there?

@danieljeans
Copy link
Contributor Author

danieljeans commented Sep 20, 2023

@tmadlener thanks for the useful comments. I've tidied up along the lines you suggested.

Copy link
Contributor

@tmadlener tmadlener left a comment

Choose a reason for hiding this comment

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

Thanks. Looks good to me now.

@tmadlener tmadlener merged commit 6fc7930 into key4hep:master Oct 4, 2023
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.

3 participants