Skip to content

Conversation

@sbaldu
Copy link
Member

@sbaldu sbaldu commented Mar 5, 2025

No description provided.

.gitignore Outdated
*.app

# build folders
*build*
Copy link
Contributor

Choose a reason for hiding this comment

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

could this be /build* instead ?

@fwyzard
Copy link
Contributor

fwyzard commented Mar 5, 2025

@sbaldu I finally merged @AuroraPerego's #1 :-)
Can you rebase your branch ?

@fwyzard
Copy link
Contributor

fwyzard commented Mar 5, 2025

Also, could you add tests for the new functions (but that's OK in a separate PR) ?

@sbaldu
Copy link
Member Author

sbaldu commented Mar 5, 2025

@sbaldu I finally merged @AuroraPerego's #1 :-) Can you rebase your branch ?

Sure

@sbaldu
Copy link
Member Author

sbaldu commented Mar 5, 2025

Also, could you add tests for the new functions (but that's OK in a separate PR) ?

Of course, I'll do it in another PR :)

.gitignore Outdated
/test/external/

# build folders
*/build/*
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused, do you expect to have .../build folders in subdirectories ?

Copy link
Member Author

@sbaldu sbaldu Mar 6, 2025

Choose a reason for hiding this comment

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

I didn't see Aurora's PR so I was expecting to use CMake for building tests as well

Copy link
Contributor

Choose a reason for hiding this comment

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

My bad, I left it pending for an year 🤷🏻

@fwyzard
Copy link
Contributor

fwyzard commented Mar 5, 2025

On second thought, can you remove the changes to .gitignore entirely ?

If the build directories come from cmake, can you move the change to .gitignore to #3 instead ?

@sbaldu
Copy link
Member Author

sbaldu commented Mar 6, 2025

On second thought, can you remove the changes to .gitignore entirely ?

If the build directories come from cmake, can you move the change to .gitignore to #3 instead ?

Of course.

@fwyzard fwyzard merged commit 15489c0 into cms-patatrack:master Mar 6, 2025
1 check passed
@VinInn
Copy link

VinInn commented Mar 6, 2025

Hope this is produced with a code generator: Could you please add the script as well?

}

XTD_DEVICE_FUNCTION
inline constexpr float acosf(float arg) {
Copy link

@VinInn VinInn Mar 6, 2025

Choose a reason for hiding this comment

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

sorry This is utterly wrong! Did you check "assembly"?
please call ::acosf at least for cuda and hip.
said that I wish to remind that both cuda and hip support templated std::acos and produce correct code.

v.

@VinInn
Copy link

VinInn commented Mar 6, 2025

-1.

Please make sure

  1. that std:: is not just enough
  2. that float16, float, double, long double and float128 are properly handled on ALL devices under consideration.
  3. please support templated code as "std"

@sbaldu
Copy link
Member Author

sbaldu commented Mar 7, 2025

Hope this is produced with a code generator: Could you please add the script as well?

I did it by hand using vim macros, which made the process relatively quick

@sbaldu
Copy link
Member Author

sbaldu commented Mar 7, 2025

-1.

Please make sure

1. that std:: is not just enough

2. that float16, float, double, long double and float128 are properly handled on ALL devices under consideration.

3. please support templated code as "std"

Thanks, I will look into all of these :)

@fwyzard fwyzard added test Issues affecting the tests documentation Improvements or additions to documentation enhancement New feature or request and removed test Issues affecting the tests documentation Improvements or additions to documentation labels Nov 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants