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

Update of calibrate measurement size should become internal to the MTJ backend #1743

Closed
tboldagh opened this issue Dec 14, 2022 · 6 comments · Fixed by #1749
Closed

Update of calibrate measurement size should become internal to the MTJ backend #1743

tboldagh opened this issue Dec 14, 2022 · 6 comments · Fixed by #1749
Assignees
Labels
Improvement Changes to an existing feature Needs Decision Needs decision or further information

Comments

@tboldagh
Copy link
Contributor

In the MTJ implementation we have such line:

calibratedSize() = measdim;

This sets the size of the calibrated measurement. However with different backend (xAOD in ATLAS) we do not need this extra size as the data is kept in vector<vector> and the size is known. We can add a fictional method to update the size but
it seems that we can eliminate this method:

template <bool RO = ReadOnly, typename = std::enable_if_t<!RO>>

and let the allocateCallibrated to handle this update.

Tagging @paulgessinger
and for info @CarloVarni Marcin Wolter

@tboldagh tboldagh added Improvement Changes to an existing feature Needs Decision Needs decision or further information labels Dec 14, 2022
@paulgessinger
Copy link
Member

So you would make calibratedSize to be purely read-only?

@tboldagh
Copy link
Contributor Author

tboldagh commented Dec 14, 2022

So you would make calibratedSize to be purely read-only?

Yes. And the update of the size handled by allocateCallibrated which would be backend dependent anyways.

@tboldagh
Copy link
Contributor Author

That was quick. Thanks. There may be another small issue here. The calibratedSize is now using "component" to get to the actual size. In ATLAS xAOD implementation this would require that we somehow get the pointer to the vector size property. Not sure if that is doable. Not sure how to best solve it. Duplicating this information in the vector & in mesdim variable seems like asking for trouble.

@paulgessinger
Copy link
Member

Oh that's a good point I didn't consider yet. I suppose we could instead special case the calibratedSize call to either call a fixed function, or change the component function to return that by value and not as a pointer.

@tboldagh
Copy link
Contributor Author

We can indeed specialise it! Nonissue then.

@paulgessinger
Copy link
Member

No I believe you're right than in the current implementation it tries to cast it to a pointer, which will break if the backend passes the size by value. So I think some form of fix is still needed.

kodiakhq bot pushed a commit that referenced this issue Dec 21, 2022
This is to facilitate backends where the calibrated size is calculated on demand, and can't easily be returned by reference.
See also #1743
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Improvement Changes to an existing feature Needs Decision Needs decision or further information
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants