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 reconstruction chain in the webApp #47

Merged
merged 1 commit into from
Oct 10, 2023
Merged

Conversation

mchiusi
Copy link
Collaborator

@mchiusi mchiusi commented Oct 8, 2023

In this PR, I have added the possibility to run the new reconstruction chain from the 3D display app, allowing users to view its outputs on the screen. Users now have the option to display cluster seeds from the new reconstruction chain, which includes the new coarse seeding and the new seeding algorithm, or from the default chain by simply interacting with a switch.

The scripts required to run the new reconstruction chain were already present in the Python framework. When calling for new events from the app, both chains are executed, so that the final user can compare the two chains applied to the same event. The new_radii_chain function has been updated accordingly. The dataframes are then processed and stored in the usual hdf5 files, where a new key has been added. This key allows for distinguishing between the new chain and the default chain and can assume the values of 'new' or 'default'.

Here's an example of a photon (200 pileup) processed by both chains. The difference in performance is truly impressive.
image
image

@mchiusi mchiusi requested a review from bfonta as a code owner October 8, 2023 16:47
@mchiusi mchiusi changed the title Add the new reconstruction chain in the webApp New reconstruction chain in the webApp Oct 8, 2023
Copy link
Owner

@bfonta bfonta left a comment

Choose a reason for hiding this comment

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

Great! General comments:

  • the key "new" for the new chain is not ideal, because as soon as we add a third chain (or a small variation of one of the current two chains) it will have to be changed
  • following the same reasoning, it is likely better to use a widget with more than two options

bye_splits/plot/display_plotly/main.py Outdated Show resolved Hide resolved
bye_splits/plot/display_plotly/plot_event.py Outdated Show resolved Hide resolved
bye_splits/plot/display_plotly/processing.py Outdated Show resolved Hide resolved
bye_splits/scripts/run_radii_chain.py Show resolved Hide resolved
bye_splits/scripts/run_radii_chain.py Outdated Show resolved Hide resolved
bye_splits/scripts/run_radii_chain.py Show resolved Hide resolved
config.yaml Outdated Show resolved Hide resolved
bye_splits/scripts/run_radii_chain.py Outdated Show resolved Hide resolved
@mchiusi
Copy link
Collaborator Author

mchiusi commented Oct 9, 2023

Great! General comments:

  • the key "new" for the new chain is not ideal, because as soon as we add a third chain (or a small variation of one of the current two chains) it will have to be changed
  • following the same reasoning, it is likely better to use a widget with more than two options

I've added a dropdown menu instead of a switch so that we can easily add multiple chains in the code. Now the new chain is called coarse seeding chain.

@bfonta
Copy link
Owner

bfonta commented Oct 9, 2023

Additional minor comments:

  • Could you please replace "t" by "T" when showing the transverse momentum?
  • Same for "Threshold in [mipₜ]", where you can also remove the "[" and "]"
  • Why are eta and phi within parenthesis and not pT? Can you uniformise?
  • I think you could add the "Select a particular clustering radius: " slider to the right of "Select manually an event:", so to gain vertical space.
  • Replace "Pages" by "View", or similar, since it is IMO clear

Once this is addressed I am ready to approve. What about @isehle?

@mchiusi
Copy link
Collaborator Author

mchiusi commented Oct 10, 2023

Additional minor comments:

  • Could you please replace "t" by "T" when showing the transverse momentum?
  • Same for "Threshold in [mipₜ]", where you can also remove the "[" and "]"
  • Why are eta and phi within parenthesis and not pT? Can you uniformise?
  • I think you could add the "Select a particular clustering radius: " slider to the right of "Select manually an event:", so to gain vertical space.
  • Replace "Pages" by "View", or similar, since it is IMO clear

Once this is addressed I am ready to approve. What about @isehle?

Implementations done! Now the page layout is a bit better!

Copy link
Owner

@bfonta bfonta left a comment

Choose a reason for hiding this comment

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

All set. Thank you for all the work, and for addressing the (many) comments.

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

2 participants