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

Add sanity checks for contiguity and orderedness #609

Merged
merged 1 commit into from
Jul 22, 2024

Conversation

stephenswat
Copy link
Member

In some places, we rely on certain properties of our inputs. For one, we rely on our measurements being contiguous (with respect to the module identifiers) in the fitting code, and we rely on our cells being ordered in some way in the clustering code. I aim to emit properly contiguous elements from the CCL code in the future, but I want to make this process foolproof. To that end, I have added two new sanity checks which aim to find problems with input. One of them checks whether elements are contiguous (according to some projection function) and the other checks whether the elements are in some order. This commit also employs these checks for debug builds in some places.

@stephenswat stephenswat added feature New feature or request cuda Changes related to CUDA labels Jun 11, 2024
stephenswat added a commit to stephenswat/traccc that referenced this pull request Jun 13, 2024
The version of clang-format that we use in this project is quite old
(version 10) and it is no longer able to properly format all of the new
stuff that has come into C++. See, for example, the formatting
nightmares in acts-project#609 and acts-project#602. This commit bumps the formatter up to
clang-format 18.
stephenswat added a commit to stephenswat/traccc that referenced this pull request Jun 13, 2024
The version of clang-format that we use in this project is quite old
(version 10) and it is no longer able to properly format all of the new
stuff that has come into C++. See, for example, the formatting
nightmares in acts-project#609 and acts-project#602. This commit bumps the formatter up to
clang-format 18.
stephenswat added a commit to stephenswat/traccc that referenced this pull request Jun 20, 2024
The version of clang-format that we use in this project is quite old
(version 10) and it is no longer able to properly format all of the new
stuff that has come into C++. See, for example, the formatting
nightmares in acts-project#609 and acts-project#602. This commit bumps the formatter up to
clang-format 18.
@stephenswat stephenswat force-pushed the feat/sanity_checks branch 2 times, most recently from 96de9ee to 1a140fd Compare July 10, 2024 13:36
stephenswat added a commit to stephenswat/traccc that referenced this pull request Jul 16, 2024
The current CCA testing code for CUDA and SYCL is missing some necessary
wait statements, which are causing me issues in acts-project#609. This commit fixes
the problem by adding the missing waits.
stephenswat added a commit to stephenswat/traccc that referenced this pull request Jul 16, 2024
The current CCA testing code for CUDA and SYCL is missing some necessary
wait statements, which are causing me issues in acts-project#609. This commit fixes
the problem by adding the missing waits.
@beomki-yeo
Copy link
Contributor

Just to check: For being contiguous, is it required that unique ID is ordered? (In the measurement vector for track finding, the order of module ID does not really matter)

@stephenswat
Copy link
Member Author

Just to check: For being contiguous, is it required that unique ID is ordered? (In the measurement vector for track finding, the order of module ID does not really matter)

Nope! It requires the IDs to be contiguous but not ordered. So an array $[5, 5, 5, 1, 1, 9, 9, 9, 9, 9, 4, 4]$ is considered contiguous, too.

Copy link
Contributor

@beomki-yeo beomki-yeo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a is_contigous_on function for CPU as well?
CPU track finding also requires measurement vector to be contiguous w.r.t module IDs

@stephenswat
Copy link
Member Author

Sure, I'll add that.

@stephenswat
Copy link
Member Author

@beomki-yeo I've added the CPU equivalents.

Copy link
Contributor

@beomki-yeo beomki-yeo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good

@stephenswat stephenswat enabled auto-merge July 22, 2024 11:13
@stephenswat stephenswat force-pushed the feat/sanity_checks branch 2 times, most recently from 9d3dfa0 to 9b309fa Compare July 22, 2024 13:19
In some places, we rely on certain properties of our inputs. For one, we
rely on our measurements being contiguous (with respect to the module
identifiers) in the fitting code, and we rely on our cells being ordered
in some way in the clustering code. I aim to emit properly contiguous
elements from the CCL code in the future, but I want to make this
process foolproof. To that end, I have added two new sanity checks which
aim to find problems with input. One of them checks whether elements are
contiguous (according to some projection function) and the other checks
whether the elements are in some order. This commit also employs these
checks for debug builds in some places.
@stephenswat stephenswat merged commit ebef105 into acts-project:main Jul 22, 2024
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cuda Changes related to CUDA feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants