Skip to content

Add function definitions for function calls involving intermediate Fastor types#162

Merged
niermann999 merged 18 commits intoacts-project:mainfrom
wermos:add-test-fix-fastor
Sep 4, 2025
Merged

Add function definitions for function calls involving intermediate Fastor types#162
niermann999 merged 18 commits intoacts-project:mainfrom
wermos:add-test-fix-fastor

Conversation

@wermos
Copy link
Contributor

@wermos wermos commented Apr 29, 2025

No description provided.

@wermos
Copy link
Contributor Author

wermos commented Apr 29, 2025

cc @niermann999 for review. This mostly is in response to what we discussed in detray PR #963.

I've added definitions for intermediate Fastor types for dot and cross so far; I also need to add one for normalize for the detray Fastor build to run.

Ideally though I should add these function definitions for all the functions, I think. What are your thoughts on the matter?

@wermos
Copy link
Contributor Author

wermos commented Apr 29, 2025

As for the tests, I've only enabled them for Fastor at the moment, because when I tried to enable it globally (for Eigen_generic, Vc_generic, and cmath_generic), it was failing for those backends.

I don't think it should be failing, but that can be addressed (if needed) in a follow-up PR.

@niermann999
Copy link
Contributor

As for the tests, I've only enabled them for Fastor at the moment, because when I tried to enable it globally (for Eigen_generic, Vc_generic, and cmath_generic), it was failing for those backends.

I don't think it should be failing, but that can be addressed (if needed) in a follow-up PR.

How exactly were the tests failing? Did they fail to compile or fail to run?

@niermann999
Copy link
Contributor

cc @niermann999 for review. This mostly is in response to what we discussed in detray PR #963.

I've added definitions for intermediate Fastor types for dot and cross so far; I also need to add one for normalize for the detray Fastor build to run.

Ideally though I should add these function definitions for all the functions, I think. What are your thoughts on the matter?

Yes, if this works, I think it should be done for all functions. Also, is it possible to remove the overloads using Fastor tensors?

@wermos
Copy link
Contributor Author

wermos commented Apr 29, 2025

How exactly were the tests failing? Did they fail to compile or fail to run?

They failed to compile. Some concept wasn't being satisfied, so it couldn't find a suitable function template to use.

I think it should be done for all functions.

Okay. I'll add them in.

Also, is it possible to remove the overloads using Fastor tensors?

I was already looking into it. The bad news is that it's not possible to remove all overloads, but the good news is that most of them should be safe to remove.

It's not possible to remove all of them because Fastor is very particular about Tensor shape. In particular, it does not allow an inner product between Fastor::Tensor<T, 3> and Fastor::Tensor<T, 3, 1> (where T is obviously float or double).

This is obviously used in certain places, and definitely tested for in the test suite. So I need to keep the overload in place that casts a Fastor::Tensor<T, 3, 1> into a Fastor::Tensor<T, 3>. But the others should be safe to remove.

@wermos
Copy link
Contributor Author

wermos commented May 3, 2025

I've rewritten the phi, theta, perp, norm, eta, and normalize functions so that they play nicely with intermediate Fastor types.

The set of overloads for dot and cross is the minimal set required to get the tests and benchmarks to compile. Removing any of the overloads causes compilation errors.

For both dot and cross, 4 overloads are required to cover all the cases:

  1. Expression template (Fastor::AbstractTensor<Derived, N>) with expression template (Fastor::AbstractTensor<Derived, N>).
  2. Pure vector (Fastor::Tensor<T, N>) with pure vector (Fastor::Tensor<T, N>).
  3. Pure vector (Fastor::Tensor<T, N>) with matrix (slice) (Fastor::Tensor<T, N, 1>).
  4. Matrix (slice) (Fastor::Tensor<T, N, 1>) with pure vector (Fastor::Tensor<T, N>).

@wermos
Copy link
Contributor Author

wermos commented May 3, 2025

Also, in order to support the intermediate types, I had to remove the requires clauses. It's not possible to determine if the type an AbstractTensor contains inside it has the required shape, at compile-time.

@wermos
Copy link
Contributor Author

wermos commented May 3, 2025

@niermann999 This PR is ready for review now.

@sonarqubecloud
Copy link

sonarqubecloud bot commented May 3, 2025

@niermann999 niermann999 merged commit a8107fd into acts-project:main Sep 4, 2025
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants