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

docs: updates to the seeding documentation #1476

Merged
merged 27 commits into from
Oct 12, 2022

Conversation

LuisFelipeCoelho
Copy link
Member

Some updates to the seeding documentation

@LuisFelipeCoelho LuisFelipeCoelho added 🚧 WIP Work-in-progress Component - Documentation Affects the documentation labels Aug 31, 2022
@LuisFelipeCoelho LuisFelipeCoelho changed the title feat: updates to the seeding documentation doc: updates to the seeding documentation Aug 31, 2022
@LuisFelipeCoelho LuisFelipeCoelho changed the title doc: updates to the seeding documentation docs: updates to the seeding documentation Aug 31, 2022
@paulgessinger paulgessinger added this to the next milestone Aug 31, 2022
@paulgessinger paulgessinger mentioned this pull request Sep 14, 2022
10 tasks
@tboldagh
Copy link
Contributor

The document starts with exclaimer that the intro section is outdated. Is it still needed? The intro is fairly general anyways.

@paulgessinger
Copy link
Member

The document starts with exclaimer that the intro section is outdated. Is it still needed? The intro is fairly general anyways.

True, with this PR that should probably be removed.

@tboldagh
Copy link
Contributor

tboldagh commented Sep 15, 2022

Can't comment o unchanged text. But I have simplifying suggestions about captions:
1 - Sketch of the detector with 3 layers. The interaction region is supposed to be located along the z axis and have size significantly smaller than the radius of the innermost detector layer.
2- The x-y projection of the detector with the charged particle helical track originating from the centre of the detector. Signals left by passage of the track through the detector layers are marked with green crosses.
3- The r-z projection of the detector with the same charged particle track. The track is depicted with the same colours as on previous figure.

If you have a chance to touch the plots can they be resized so that on each of them the detector has similar size.
On the figure 3 I would suggest to mark the radius of the detectror as done in 2.

In general the explanations do not belong to the picture captions but should be part of the text.

The explanation of the algorithm starts with phi-z projection. Maybe drawing of such projection could be useful?

docs/core/seeding.rst Outdated Show resolved Hide resolved
docs/core/seeding.rst Outdated Show resolved Hide resolved
docs/core/seeding.rst Outdated Show resolved Hide resolved
docs/core/seeding.rst Outdated Show resolved Hide resolved
docs/core/seeding.rst Outdated Show resolved Hide resolved
docs/core/seeding.rst Outdated Show resolved Hide resolved
docs/core/seeding.rst Outdated Show resolved Hide resolved
docs/core/seeding.rst Outdated Show resolved Hide resolved
docs/core/seeding.rst Outdated Show resolved Hide resolved
docs/core/seeding.rst Outdated Show resolved Hide resolved
@LuisFelipeCoelho
Copy link
Member Author

Thanks a lot @tboldagh for your comments! I will update this ASAP

@LuisFelipeCoelho
Copy link
Member Author

@tboldagh @paulgessinger
I hope that I have addressed all the comments.
I have decided to replace the code block with some equations and further explanation. If you think that I have added too much information or that the pseudo code is still necessary or that I need to add more information, please tell me and I will change it accordingly.

docs/core/seeding.md Outdated Show resolved Hide resolved
docs/core/seeding.md Outdated Show resolved Hide resolved
docs/core/seeding.md Outdated Show resolved Hide resolved
docs/core/seeding.md Outdated Show resolved Hide resolved
docs/core/seeding.md Outdated Show resolved Hide resolved
LuisFelipeCoelho and others added 3 commits October 3, 2022 10:17
Co-authored-by: Paul Gessinger <hello@paulgessinger.com>
Co-authored-by: Paul Gessinger <hello@paulgessinger.com>
Co-authored-by: Paul Gessinger <hello@paulgessinger.com>
@LuisFelipeCoelho
Copy link
Member Author

not sure why the docs are failing

@paulgessinger
Copy link
Member

I think its

/__w/acts/acts/docs/core/seeding.md:190: WARNING: doxygenfunction: Cannot find function "SeedFilter::filterSeeds_2SpFixed" in doxygen xml output for project "Acts" from directory: _build/doxygen-xml
/__w/acts/acts/docs/core/seeding.md:226: WARNING: doxygenfunction: Cannot find function "SeedFilter::filterSeeds_1SpFixed" in doxygen xml output for project "Acts" from directory: _build/doxygen-xml

I'm guessing you need to add the Acts:: namespace before the functions?

@LuisFelipeCoelho LuisFelipeCoelho removed the 🚧 WIP Work-in-progress label Oct 7, 2022
docs/core/seeding.md Outdated Show resolved Hide resolved
docs/core/seeding.md Outdated Show resolved Hide resolved
docs/core/seeding.md Outdated Show resolved Hide resolved
LuisFelipeCoelho and others added 5 commits October 7, 2022 10:41
Co-authored-by: Alexander J. Pfleger <70842573+AJPfleger@users.noreply.github.com>
Co-authored-by: Alexander J. Pfleger <70842573+AJPfleger@users.noreply.github.com>
Co-authored-by: Alexander J. Pfleger <70842573+AJPfleger@users.noreply.github.com>
@LuisFelipeCoelho
Copy link
Member Author

Thanks a lot @AJPfleger for your comments

@benjaminhuth benjaminhuth self-assigned this Oct 11, 2022
Copy link
Member

@benjaminhuth benjaminhuth left a comment

Choose a reason for hiding this comment

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

I just read this seeding chapter and found it really good and descriptive, with nice figures, really good work. I think this can go in!

@LuisFelipeCoelho
Copy link
Member Author

Thanks for your revision @benjaminhuth

@kodiakhq kodiakhq bot merged commit ecf90a3 into acts-project:main Oct 12, 2022
@LuisFelipeCoelho LuisFelipeCoelho deleted the seeding-doc-update branch October 12, 2022 07:25
@LuisFelipeCoelho LuisFelipeCoelho self-assigned this Oct 12, 2022
kodiakhq bot pushed a commit that referenced this pull request Oct 14, 2022
By mistake I forgot to add a caption to one of the figures from #1476
@paulgessinger paulgessinger modified the milestones: next, v21.0.0 Nov 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component - Documentation Affects the documentation
Projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants