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

Files for validation pipeline #3

Open
wants to merge 76 commits into
base: master
Choose a base branch
from

Conversation

enlupi
Copy link

@enlupi enlupi commented Sep 4, 2024

This pull request includes the following changes:

  • ADD scripts/FCCee directory, which includes:
    - bash and analysis scripts for ALLEGRO, CLD, and IDEA in the respective directories
    - common utility scripts for comparing and plotting histograms in the utils directory

  • Modified the www and web directories to fix the website for the new validation pipeline.
    - modified web/python/make_web.py and web/templates to adapt to the new website structure
    - Added www/static/styles.css for new website look

scripts/FCCee/ALLEGRO/ALLEGRO_o1_v03/ALLEGRO_make_TH1.py Outdated Show resolved Hide resolved
Comment on lines 34 to 39
#############################################################################
## ##
## BEGIN: ECal Barrel Histogram Definition ##
## ##
#############################################################################

Copy link
Member

Choose a reason for hiding this comment

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

I'm not a fan of these blocks, if you keep reading then it's obvious so it's not telling you anything. If you want a small comment so that you can jump, it can be just # Beginning of ECal Barrel and the ending has Endso that the in the editor you can search for Begin and End. In one of the files below, the blocks look to be almost 50% of the contents of the script.

Choose a reason for hiding this comment

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

It might also be a simple sign that you effectively have (almost) a function (or at least something that can be refactored into one) here. In that case you solve several issues in one step:

  • You have a clearly defined name for searching
  • You have a very well defined scope of where it starts and ends
  • You have a clearly defined place where you can put some top level comments / a docstring
  • Most editors allow you to fold / collapse the function body, so readability of the complete file also improves

Copy link
Author

Choose a reason for hiding this comment

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

I put them because Brieuc requested them: we wanted clearly separated sections for each subdetector so people could easily add new sections or modify the ones they were responsible for without disturbing the rest of the script. I can try to make them smaller if you think that would help, but i would not get rid of them completely.

Encapsulating them into functions is also an option, let me know what would be preferred.

Copy link

@tmadlener tmadlener Sep 11, 2024

Choose a reason for hiding this comment

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

I would have a preference for functions, as that minimizes chances that changes in one "section" influence anything somewhere else.

edit: But in the end it's also up to you a bit and how much time you can / want to invest into going down that route.

Comment on lines 89 to 90
energy = calo.getEnergy()
hist_ccE.Fill(energy)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
energy = calo.getEnergy()
hist_ccE.Fill(energy)
hist_ccE.Fill(calo.getEnergy())

It's easier to read and the intermediate variable isn't doing anything. Same comment for other lines.



# simulation phase
printf "\n\nSIM-DIGI-RECO PHASE:\n"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
printf "\n\nSIM-DIGI-RECO PHASE:\n"
echo "\n\nSIM-DIGI-RECO PHASE:\n"

Use only echo for consistency. Same comment for the other occurrences of printf.

if isinstance(v, str) or len(v) == 1:
metadata[k] = [v, None]
print(metadata)
def get_metadata(folder_path):
Copy link
Member

Choose a reason for hiding this comment

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

Now that I remember it, for metadata it would be fine to remove the key4hep-spack and spack commits; it's not necessary for users to know it and it's already bundled in the stack, so knowing which stack was used lets you know which commits were used. When I wrote this initially, the information of the commits wasn't available in each stack.

jmcarcell
jmcarcell approved these changes Sep 10, 2024
Copy link
Member

@jmcarcell jmcarcell left a comment

Choose a reason for hiding this comment

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

I added some style and minor comments on the code. I recommend using a python formatter like ruff or black, both are available in the stack. Then I have some other comments:

  • Each geometry having a script to make plots is OK but I feel there is too much repetition but I'm not sure if parts can be abstracted.
  • With the new layout space is not used well when there are multiple plots as all of them are in a vertical column.
  • There isn't much documentation, for example, what does an expert have to do if they want to add a detector, subdetector or plot to the webpage?

@enlupi
Copy link
Author

enlupi commented Sep 11, 2024

I saw the changes you suggested, and I agree with most of them.

The most pressing issue now is adding the documentation. Luckily, I have already written a README for my own personal repository that contains all the information you requested: I will update it with the latest changes and submit it asap.

The other issues are mostly "cosmetical", so I will have to give priority to other mandatory tasks like finishing the report before addressing them.

@atolosadelgado
Copy link

I added some style and minor comments on the code. I recommend using a python formatter like ruff or black, both are available in the stack. Then I have some other comments:

* Each geometry having a script to make plots is OK but I feel there is too much repetition but I'm not sure if parts can be abstracted.

* With the new layout space is not used well when there are multiple plots as all of them are in a vertical column.

* There isn't much documentation, for example, what does an expert have to do if they want to add a detector, subdetector or plot to the webpage?

Hi Juan

  1. You are right about the repetition, but since the same subdetector can be used by different detector concepts and options, I do not see how to abstract that without adding a layer of complexity to it. For example, although the Geant4 simulation uses the full detector, the reconstruction and analysis scripts can be defined for individual subdetectors and then called depending on the detector model). Since the bash script is living somewhere else (FCCconfig), the abstraction should be proposed there right?
  2. The layout of the histograms could be tackled in a subsequent PR or do you consider it a showstopper?

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.

4 participants