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

adding docker part and isolated muon example #68

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

EnginEren
Copy link

BEGINRELEASENOTES

  • Adding docker image for k4SimDelphes
  • Case study example: Invariant mass of isolated muons

ENDRELEASENOTES

@vvolkl
Copy link
Contributor

vvolkl commented Feb 8, 2022

Thanks! You could save some bytes in the docker image by removing the build folders after installing - they don't need to be distributed in the image, right?

@EnginEren
Copy link
Author

Good point! Let me try

@EnginEren
Copy link
Author

Done. I have saved like 300MB.

@vvolkl
Copy link
Contributor

vvolkl commented Feb 10, 2022

I think the notebook would benefit from a plain text description what it is doing and the plots from some axis labels. In general plain markdown files are nicer to use with version control and can be converted to notebooks with tools like jupytext, see what we are doing here: https://github.com/key4hep/k4MarlinWrapper/blob/master/doc/starterkit/k4MarlinWrapperCLIC/Readme.md
but I'd also merge the .ipynb if it is a bit more verbose.

Just out of curiosity: Why use unpacked.cern.ch when k4simdelphes is regularly installed to /cvmfs/sw.hsf.org/? Isn't it easier to just mount cvmfs in a plain centos7 docker image?

@EnginEren
Copy link
Author

Hi Valentin,

Sounds good to me. I have converted it.

Regarding unpacked.cern.ch, it gives us the flexibility to use custom or official docker images with singularity. The main idea is to avoid using

singularity shell docker://ilcsoft/k4simdelphes:latest 

as this will pull the image and unpack it in your working directory. This takes like 30min. Of course, it's fast after caching. More information is here

@EnginEren
Copy link
Author

Hi @vvolkl and @tmadlener
what's currently blocking the merge? I'm happy to implement if you have other wishes/suggestion?

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.

Hi @EnginEren,

sorry for the delay. I have a few minor comments about some details.

docker/Dockerfile Outdated Show resolved Hide resolved
docker/Dockerfile Outdated Show resolved Hide resolved
docker/Dockerfile Outdated Show resolved Hide resolved
docker/README.md Outdated Show resolved Hide resolved
examples/README.md Outdated Show resolved Hide resolved
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