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

Evtgenfix #35

Closed
wants to merge 23 commits into from
Closed

Evtgenfix #35

wants to merge 23 commits into from

Conversation

clementhelsens
Copy link
Contributor

BEGINRELEASENOTES

@clementhelsens
Copy link
Contributor Author

need to clean this PR, did not paid attention pythia stuff where also pushed

@clementhelsens clementhelsens changed the title Evtgenfix [WIP] Evtgenfix Apr 15, 2021
@clementhelsens clementhelsens changed the title [WIP] Evtgenfix Evtgenfix Apr 15, 2021
@clementhelsens
Copy link
Contributor Author

Done with cleaning

@vvolkl
Copy link
Contributor

vvolkl commented Apr 29, 2021

There should ideally be a test for the issue you mention, but that'll take some time, so I created an issue to do this at a future date. Could you still rebase and remove the whitespace changes?

{"ScalarHT"});
{""});
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure you need to change the converter, maybe by accident?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was a local change indeed that I reverted. If we don't remove the hardcoded names in the converter, there is not the possibility to not save a collection by removing it from the output_config.tcl

Copy link
Contributor

Choose a reason for hiding this comment

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

I think these are fixing a different kind of "problem". Without these changes, the converter will always fill these collections, even if you remove them from your output config file. I.e. the defaults here might override your configuration. Without these fixes you have to explicitly touch the delphes card and remove the branches you do not want from the TreeWriter config section. With these changes you can leave the delphes card untouched and simply suppress these outputs by removing them from the output configuration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

BTW @tmadlener , using the default configuration for the output I do not see ScalarHT in the output root files.
I never noticed it because I am not using it until now while I was testing the collection removal

Copy link
Contributor

Choose a reason for hiding this comment

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

That might be because ScalarHT is not in the TreeWriter configuration? By default the converter will only try to convert what is actually there.

@clementhelsens
Copy link
Contributor Author

thanks @vvolkl , I also made the verbose flag to false.

@clementhelsens
Copy link
Contributor Author

is there anything before we merge this?
I need this PR in for the next release so that I can based the first official FCC-ee production on it

vvolkl added a commit to vvolkl/k4SimDelphes that referenced this pull request Apr 29, 2021
*    Fixes a bug with charge conjugate decays in EvtGen
*    Found by @clementhelsens and fixed by @mchrzasz

Co-authored-by: Clement Helsens <clement.helsens@cern.ch>
@vvolkl
Copy link
Contributor

vvolkl commented Apr 29, 2021

It cannot be rebased, but I'll merge this changes in #40

@vvolkl vvolkl closed this Apr 29, 2021
vvolkl added a commit that referenced this pull request Apr 29, 2021
*    Fixes a bug with charge conjugate decays in EvtGen
*    Found by @clementhelsens and fixed by @mchrzasz

Co-authored-by: Clement Helsens <clement.helsens@cern.ch>
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