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

Override getParameters for main #905

Merged
merged 1 commit into from
Feb 28, 2024
Merged

Override getParameters for main #905

merged 1 commit into from
Feb 28, 2024

Conversation

anigamova
Copy link
Collaborator

Same as #903, provide a fix for discrete profiling in root v6.26

@kcormi kcormi merged commit 99c15de into main Feb 28, 2024
9 checks passed
@guitargeek
Copy link
Contributor

Thanks for this! There are some issues with it still:

  • the added function leaks memory (the return value of the getParameters called internally)
  • the fix also needs to be applied to CachingAddNLL
  • it's a bit risky to overload both the old and new getParameters signature at the same time. Depending on how ROOT implements things, there might be infinite recursion

All this this is not your fault: I should have been more careful when changing interfaces, and combine should have used the override keyword like the rest of CMSSW to catch this early.

Sustainable fix proposed in:
#948

@anigamova
Copy link
Collaborator Author

Thanks @guitargeek for looking into this and providing a better fix for this issue. We'll test the changes from #948 and hopefully merge it soon

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants