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

ref: remove b-field from detector #566

Merged
merged 4 commits into from
Oct 18, 2023

Conversation

niermann999
Copy link
Contributor

@niermann999 niermann999 commented Oct 11, 2023

This PR removes the bfield from the detector and decouples detray core from covfie. The management of the bfield (building/moving to device) is done in the client code (e.g. the detray unittests etc).

Note: this does not remove the templating/need for explicit template instantiation on the propagation kernels in detray, it simply allows to find a different solution in traccc

@niermann999 niermann999 force-pushed the ref-remove-bfield-from-det branch from 8a818a3 to 8e37a0f Compare October 11, 2023 16:45
@niermann999 niermann999 marked this pull request as ready for review October 11, 2023 16:48
@niermann999 niermann999 added refactor refactoring the current codes priority: high high priority labels Oct 11, 2023
@niermann999 niermann999 force-pushed the ref-remove-bfield-from-det branch 6 times, most recently from 154f0ad to 405ad1e Compare October 12, 2023 14:44
@beomki-yeo
Copy link
Collaborator

I tried to solve the warning of calling a __host__ function from a __host__ __device__ function is not allowed but I really cannot locate what is causing the problem. I guess it is on vecmem::vector being used in device code - but I could not find such case. @niermann999 Any thoughts?

@niermann999
Copy link
Contributor Author

I tried to solve the warning of calling a __host__ function from a __host__ __device__ function is not allowed but I really cannot locate what is causing the problem. I guess it is on vecmem::vector being used in device code - but I could not find such case. @niermann999 Any thoughts?

No, I also tried to find it and failed. Additionally, it is complaining about some variables that are set but not referenced in the toy geometry creation, which is clearly incorrect. I am thinking that I have redo the whole thing, but starting with replacing the bfield before removing the template and trying to go step by step (as much as that is possible with this change)

@beomki-yeo
Copy link
Collaborator

Additionally, it is complaining about some variables that are set but not referenced in the toy geometry creation

That's possibly a nvcc bug or at least a warning that can be ignored.
The problem was that create_toy_geometry.hpp is included in header file which is also compiled by nvcc.
Header file usually defines detector_host_type so lots of functions should be defined for vecmem::vector which is not allowed by Cuda compiler

@niermann999
Copy link
Contributor Author

Ok, thanks for having a look. The warning about the unreferenced variables should vanish, too, since it was in create_toy_geometry as well. Otherwise, we need to disable it, since every warning will block the CI

@niermann999
Copy link
Contributor Author

Odd, that these warnings did not come up before...

@beomki-yeo
Copy link
Collaborator

That's odd indeed. I also don't know what triggered this error.

@beomki-yeo
Copy link
Collaborator

I could not fix the narrowing conversion warning and ended up with disabling detray propagation tutorial for the moment.

Because I need to update detray version and move on to traccc work - @niermann999 Could you kindly accept this?

@beomki-yeo beomki-yeo force-pushed the ref-remove-bfield-from-det branch from 546d630 to 0e2a257 Compare October 18, 2023 08:49
@niermann999 niermann999 force-pushed the ref-remove-bfield-from-det branch 3 times, most recently from d43e8fd to e5f2314 Compare October 18, 2023 10:13
@niermann999
Copy link
Contributor Author

I could not fix the narrowing conversion warning and ended up with disabling detray propagation tutorial for the moment.

Reverted back to using a const field in the tutorials, that fixed the issue for me (at least locally). Then let's go with this

@niermann999 niermann999 force-pushed the ref-remove-bfield-from-det branch from e5f2314 to 8b5dbdc Compare October 18, 2023 11:03
@beomki-yeo
Copy link
Collaborator

Yeah but let's not spend too much time for something we don't fully understand. We don't know if the covfie::cuda warning is from detray or covfie itself

@niermann999
Copy link
Contributor Author

It seems to come from covfie, it happens during the compilation of the detray bfield header, even if the inhomogeneous type and the bfield reader are not used. But OK, if you need this asap, I will fix it in the next PR

@niermann999 niermann999 force-pushed the ref-remove-bfield-from-det branch from 8b5dbdc to 263d79b Compare October 18, 2023 11:30
@beomki-yeo
Copy link
Collaborator

I cannot do anything without this. so yes I need this ASAP.

@beomki-yeo
Copy link
Collaborator

beomki-yeo commented Oct 18, 2023

BTW you will also need to disable detector construction.

I forgot to mention that the compilation fails even without covife::cuda

@beomki-yeo beomki-yeo force-pushed the ref-remove-bfield-from-det branch from 9e0d28e to 842a31d Compare October 18, 2023 12:51
@beomki-yeo
Copy link
Collaborator

@niermann999 I removed the covfie library from detector construction

@niermann999
Copy link
Contributor Author

Ok, I am trying to finish the rest in #568 . Let's merge this

@niermann999
Copy link
Contributor Author

niermann999 commented Oct 18, 2023

I think you need to remove the bfield header in the detector construction, but let's just comment the tutorial out for now

@beomki-yeo
Copy link
Collaborator

I commented them now.

@beomki-yeo beomki-yeo merged commit 8b3e0d5 into acts-project:main Oct 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: high high priority refactor refactoring the current codes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants