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

Remove all plane-related codes w/ SST #552

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

wcmartylee
Copy link
Contributor

Remove all plane-related codes due to implementation of compact (planeless) strawtubes configuration

@wcmartylee wcmartylee requested a review from a team as a code owner November 4, 2024 15:45
@olantwin
Copy link
Contributor

olantwin commented Nov 5, 2024

Thanks for this pull request!

Could you please prepare a presentation for Monday about these changes?

Some observations in no particular order:

  • Currently there is a lot of hard-coding of detector IDs and conversions. We should probably have a set of functions to decode the ID instead of doing it by hand all over the place. (probably as part of the strawtubespoint/hit so that there is no desynchronisation between data and the way to decode the detector IDs)
  • I don't like the multiple-return-by-reference style of the current decoding function. Would be nice to modernise this by e.g. returning a pair/tuple
  • I assume this change will be very backwards incompatible, we should have a look how much breaks and whether there is a way to make the transition as simple as possible given reasonable effort
  • The commit history is a mess
  • At least the touched lines should be auto-formatted
  • There is no need to modify the muflux files. I must have forgotten to remove them!

@wcmartylee
Copy link
Contributor Author

@olantwin
Thanks for the review! Of course, I can give a presentation on the changes next Monday.

Some thoughts while I was midway of changing the detID: maybe it’s a lot easier and “safer” to leave the plane number 0 as it is. But either way, you’re right that we should have a separate function for construct and destruct. It was a pain in the ass to change them by hand.

And I’ll try to fix the commit history. (It was a learning-by-doing situation...)

@olantwin
Copy link
Contributor

olantwin commented Nov 5, 2024

@olantwin Thanks for the review! Of course, I can give a presentation on the changes next Monday.

Great, I'll put you onto the agenda.

Some thoughts while I was midway of changing the detID: maybe it’s a lot easier and “safer” to leave the plane number 0 as it is. But either way, you’re right that we should have a separate function for construct and destruct. It was a pain in the ass to change them by hand.

Maybe we can do two steps: move the detID encoding/decoding to a function and then in a second step change the detector ID calculation once, in a single spot while also incrementing the version of the strawtubespoint/hit

And I’ll try to fix the commit history. (It was a learning-by-doing situation...)

No worries, 99% of what I know about git I learnt experimentally by repeatedly getting myself into trouble ;)

@olantwin
Copy link
Contributor

Could you please rebase to remove the merge conflicts? They result from deleted files which didn't need updating.

@wcmartylee
Copy link
Contributor Author

Could you please rebase to remove the merge conflicts? They result from deleted files which didn't need updating.

Just did!

@olantwin
Copy link
Contributor

I think it might have been rebase the wrong way around, the Remove last vestiges of muflux
6b9bde8 commit should precede your changes

@olantwin
Copy link
Contributor

@wcmartylee Any updates on this PR?

@wcmartylee
Copy link
Contributor Author

I’m sorry... I was busy with other things and couldn’t finish this fix. I was hoping we can complete this quickly on next week’s hackathon.

@olantwin
Copy link
Contributor

Indeed, for this kind of thing the hackathon should work well

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.

2 participants