Skip to content

Configurability using config file#3

Merged
Zehvogel merged 33 commits intokey4hep:mainfrom
Victor-Schwan:main
Jun 19, 2024
Merged

Configurability using config file#3
Zehvogel merged 33 commits intokey4hep:mainfrom
Victor-Schwan:main

Conversation

@Victor-Schwan
Copy link
Copy Markdown
Contributor

@Victor-Schwan Victor-Schwan commented May 31, 2024

BEGINRELEASENOTES

  • Paths to personal and detector-specific files/folders outsourced into the config file
  • Choose config file with argparse
  • Path operations using pathlib
  • so far, only tested with condorJobs_sim.py

ENDRELEASENOTES

@Zehvogel Zehvogel self-assigned this May 31, 2024
Comment thread TrackingPerformance/Condor/condorJobs_sim.py Outdated
Copy link
Copy Markdown
Collaborator

@Zehvogel Zehvogel left a comment

Choose a reason for hiding this comment

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

Thx!

Some comments below. I did not look much at the other files yet.

Comment thread TrackingPerformance/Condor/condorJobs_sim.py Outdated
Comment thread TrackingPerformance/Condor/condorJobs_sim.py Outdated
Comment thread TrackingPerformance/Condor/condorJobs_sim.py Outdated
Comment thread TrackingPerformance/Condor/condorJobs_sim.py Outdated
Comment thread TrackingPerformance/Condor/condorJobs_sim.py
Comment thread TrackingPerformance/Condor/condorJobs_reco.py Outdated
@Zehvogel
Copy link
Copy Markdown
Collaborator

Zehvogel commented Jun 5, 2024

@Victor-Schwan is this ready now?

@Victor-Schwan
Copy link
Copy Markdown
Contributor Author

Not ready yet. When I ran it on lxbatch there was a permission problem in sim_jobs which needs to be debugged. And I have an almost completed commit waiting removing double definitions of vars and some other refacs

@Zehvogel
Copy link
Copy Markdown
Collaborator

Zehvogel commented Jun 5, 2024

Ok

now only definied in config file and not anymore again in scripts;
integer variables defined as `ints` and not `strings`:
unnecessary type casting removed
@Victor-Schwan Victor-Schwan marked this pull request as ready for review June 11, 2024 12:40
Copy link
Copy Markdown
Collaborator

@Zehvogel Zehvogel left a comment

Choose a reason for hiding this comment

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

Some more comments.

I guess we will need a future PR on simJobs_reco to make it a bit more flexible but for now I am happy.

I will merge after @gaswk approves. :)

Comment thread TrackingPerformance/Condor/condorJobs_reco.py
Comment thread TrackingPerformance/Condor/condorJobs_reco.py Outdated
Comment thread TrackingPerformance/Condor/condorJobs_sim.py Outdated
Comment thread TrackingPerformance/Condor/condorJobs_reco.py Outdated
Comment thread TrackingPerformance/Condor/condorJobs_reco.py
Comment thread TrackingPerformance/Condor/condorJobs_reco.py Outdated
@Zehvogel
Copy link
Copy Markdown
Collaborator

@gaswk is everything fine from your side now?

@nitasad
Copy link
Copy Markdown
Collaborator

nitasad commented Jun 19, 2024

@gaswk is everything fine from your side now?

I am waiting for your response to my previous comment regarding the naming conventions for the output files. To maintain consistency, should we use _ instead of . for all output names? Currently, the outputs from the reco are named _edm4hep.root and _aida.root by default.

@Zehvogel
Copy link
Copy Markdown
Collaborator

I am waiting for your response to my previous comment regarding the naming conventions for the output files. To maintain consistency, should we use _ instead of . for all output names? Currently, the outputs from the reco are named _edm4hep.root and _aida.root by default.

Oops, sorry for missing that. I would prefer . instead of _ but if CLDConfig still produces _ we would need to change it there first of course :/ I think a have a change for that ready somewhere but that also changes some more parts.

@Victor-Schwan opposite to previous instructions I would then suggest that you change back to _ for this PR and we make the change to . later...

@Victor-Schwan
Copy link
Copy Markdown
Contributor Author

Victor-Schwan commented Jun 19, 2024

I added a bool var in the config file which switches between _edm4hep.root and .edm4hep.root paths. What do you think? @Zehvogel @gaswk

Copy link
Copy Markdown
Collaborator

@Zehvogel Zehvogel left a comment

Choose a reason for hiding this comment

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

I think you meant this?

Comment thread TrackingPerformance/config_template.py Outdated
Comment thread TrackingPerformance/config_v.py Outdated
@nitasad
Copy link
Copy Markdown
Collaborator

nitasad commented Jun 19, 2024

Everything is fine for me now!

@Zehvogel Zehvogel merged commit fd4d078 into key4hep:main Jun 19, 2024
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