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

[RF] Add RooArgSet storage to RooAbsData for GlobalObservables #8123

Closed
will-cern opened this issue May 7, 2021 · 14 comments · Fixed by #8878
Closed

[RF] Add RooArgSet storage to RooAbsData for GlobalObservables #8123

will-cern opened this issue May 7, 2021 · 14 comments · Fixed by #8878

Comments

@will-cern
Copy link
Contributor

It would be very helpful if RooAbsData was able to store an optional associated RooArgSet (simply as a data member?) of global observable values. These should be settable by the user.

Once this is added, the methods like 'fitTo' could automatically make use of this set for the global observables. Also it would make sense that 'getParameters' considered these global observables to be observables and so getParameters doesn't return the global observables in the list.

Thanks!

@guitargeek
Copy link
Contributor

Hi @will-cern! I opened a PR that implements what you need (I think). Can you please have a look and let us know if this is what you meant?

#8156

If you think this is a good start, we can continue the discussion with @lmoneta.

@guitargeek guitargeek assigned guitargeek and unassigned lmoneta May 11, 2021
guitargeek added a commit to guitargeek/root that referenced this issue May 11, 2021
@will-cern
Copy link
Contributor Author

Thanks. I've had a quick look -- so your approach is to say that the global observables get added to the dataset in the same way you would list your observables, but then you have a way to flag which of the observables are the global ones?

I guess I was imagining instead that the global observables would be truly independent of the observables. So e.g. I would have code like:

RooDataSet ds("data","data", RooArgSet{a,b,c}, RooFit::GlobalObservables(RooArgSet({x,y,z}));
ds.getGlobals(); // return RooArgSet* to global observables, or null pointer if Dataset has no global observables attached
ds.getGlobals()->setRealValue("x",4); // changing the value in the global observables

Do you think that sort of interface will cause problems though?

@guitargeek
Copy link
Contributor

Hi, thanks for your comment! Hmm I have to think about that suggestion with the constructor. It would be nice certainly, but adding a parameter to the constructor of an abstract class means usually a lot of code repetition: the parameter would have to be added to most constructors of all child classes (RooDataSet and RooDataHist). Maybe we also need a configuration struct here.

Ah right, I missed that! The global observables are usually not even in the columns of the dataset, right? So my assumption that they are a subset of the columns was just plain wrong, no?

@will-cern
Copy link
Contributor Author

will-cern commented Jul 22, 2021

Just thought I'd poke this issue @guitargeek because it came up in a meeting again today.

Have we settled on whether the global observables should be part of the collection returned by get() or not? I think it would be sensible that you cannot modify the global observables, so I guess have their values set at the time the dataset is constructed. I definitely think it would be good to have:

RooDataSet d("d","d",RooArgSet(obs1, obs2, ..), RooFit::GlobalObservables(RooArgSet(globs1,globs2...)) )

To create a dataset with global observables attached. I guess it would be fine for d.get() to return the obs along with the globs, perhaps flagging which which are the globals with a "global" attribute ? so e.g. user can do:

d.get()->selectByAttrib("global",true)

to pick out the global observables. Otherwise just d.getGlobals() would be fine if its easier to keep the obs separated from the globs

@github-actions
Copy link

Hi @guitargeek,

It appears this issue is closed, but wasn't yet added to a project. Please add upcoming versions that will include the fix, or 'not applicable' otherwise.

Sincerely,
🤖

9 similar comments
@github-actions
Copy link

Hi @guitargeek,

It appears this issue is closed, but wasn't yet added to a project. Please add upcoming versions that will include the fix, or 'not applicable' otherwise.

Sincerely,
🤖

@github-actions
Copy link

Hi @guitargeek,

It appears this issue is closed, but wasn't yet added to a project. Please add upcoming versions that will include the fix, or 'not applicable' otherwise.

Sincerely,
🤖

@github-actions
Copy link

Hi @guitargeek,

It appears this issue is closed, but wasn't yet added to a project. Please add upcoming versions that will include the fix, or 'not applicable' otherwise.

Sincerely,
🤖

@github-actions
Copy link

Hi @guitargeek,

It appears this issue is closed, but wasn't yet added to a project. Please add upcoming versions that will include the fix, or 'not applicable' otherwise.

Sincerely,
🤖

@github-actions
Copy link

Hi @guitargeek,

It appears this issue is closed, but wasn't yet added to a project. Please add upcoming versions that will include the fix, or 'not applicable' otherwise.

Sincerely,
🤖

@github-actions
Copy link

Hi @guitargeek,

It appears this issue is closed, but wasn't yet added to a project. Please add upcoming versions that will include the fix, or 'not applicable' otherwise.

Sincerely,
🤖

@github-actions
Copy link

Hi @guitargeek,

It appears this issue is closed, but wasn't yet added to a project. Please add upcoming versions that will include the fix, or 'not applicable' otherwise.

Sincerely,
🤖

@github-actions
Copy link

Hi @guitargeek,

It appears this issue is closed, but wasn't yet added to a project. Please add upcoming versions that will include the fix, or 'not applicable' otherwise.

Sincerely,
🤖

@github-actions
Copy link

Hi @guitargeek,

It appears this issue is closed, but wasn't yet added to a project. Please add upcoming versions that will include the fix, or 'not applicable' otherwise.

Sincerely,
🤖

@guitargeek guitargeek changed the title Add RooArgSet storage to RooAbsData for GlobalObservables [RF] Add RooArgSet storage to RooAbsData for GlobalObservables Jun 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants