Skip to content

Move nlambda default behavior into base class #489

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

Merged
merged 22 commits into from
Apr 10, 2025
Merged

Move nlambda default behavior into base class #489

merged 22 commits into from
Apr 10, 2025

Conversation

jhp-lanl
Copy link
Collaborator

@jhp-lanl jhp-lanl commented Apr 9, 2025

PR Summary

Does what it says...

PR Checklist

  • N/A Adds a test for any bugs fixed. Adds tests for new features.
  • Format your changes by using the make format command after configuring with cmake.
  • N/A Document any new features, update documentation for changes made.
  • Make sure the copyright notice on any files you modified is up to date.
  • After creating a pull request, note it in the CHANGELOG.md file.
  • LANL employees: make sure tests pass both on the github CI and on the Darwin CI

If preparing for a new release, in addition please check the following:

  • Update the version in cmake.
  • Move the changes in the CHANGELOG.md file under a new header for the new release, and reset the categories.
  • Ensure that any when='@main' dependencies are updated to the release version in the package.py

@jhp-lanl
Copy link
Collaborator Author

jhp-lanl commented Apr 9, 2025

Note: I didn't include a using statement in the SG_ADD_BASE_CLASS_USINGS macro because the intention is to hide the nlambda function from the base class when there are more than zero lambdas. With a using statement, the function becomes ambiguously defined rather than clearly shadowed.

We have more of these cases in the code where it might be useful to hide the base class version of a function, but we wouldn't be able to do so because of excessive using statements. That said, I think it's probably fine to just leave them until we want to hide or shadow the base class version with a different implementation in the derived class.

@jhp-lanl jhp-lanl requested a review from Yurlungur April 9, 2025 01:35
Copy link
Collaborator

@Yurlungur Yurlungur left a comment

Choose a reason for hiding this comment

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

Thanks for the cleanup! For the EOS's that still have an nlambda, we need to make sure the signature is identical. That is to say, they need to be static int nlambda() noexcept now.

@jhp-lanl
Copy link
Collaborator Author

jhp-lanl commented Apr 9, 2025

I'll double check. Since I'm changing things, @Yurlungur what are your thoughts on just making these all constexpr static inline instead of using the PORTABLE_INLINE_FUNCTION decorator?

@Yurlungur
Copy link
Collaborator

If it works on device, I'm fine with that. If I recall, we were bitten by constexpr not working on device as we expected previously.

@jhp-lanl
Copy link
Collaborator Author

jhp-lanl commented Apr 9, 2025

From my reading, if the expression is compile time constant, it should be inlined and will be available on device. I can't think of a reason why this function wouldn't be compile time constant.

@Yurlungur
Copy link
Collaborator

Well lets just give it a try and see if tests pass. (May need to add a test to stress this)

@jhp-lanl
Copy link
Collaborator Author

jhp-lanl commented Apr 9, 2025

Well lets just give it a try and see if tests pass. (May need to add a test to stress this)

I ran into an issue with the Variant and the modifers. The main issue was that the EOS object isn't constexpr. In the modifier case, I could have called the static version from the type rather than the EOS object itself, but I felt like I was battling too much here.

@jhp-lanl
Copy link
Collaborator Author

jhp-lanl commented Apr 9, 2025

Okay @Yurlungur I think everything should be the same now. I think I could have made static or constexpr static work, but it would have meant that the Variant versions would not be constexpr while the EOS objects would (and the modifers would have to use the type rather than the EOS object). I think I'll just stick with what works for now. Hopefully the compiler can still inline most of this

@Yurlungur
Copy link
Collaborator

Okay @Yurlungur I think everything should be the same now. I think I could have made static or constexpr static work, but it would have meant that the Variant versions would not be constexpr while the EOS objects would (and the modifers would have to use the type rather than the EOS object). I think I'll just stick with what works for now. Hopefully the compiler can still inline most of this

Sounds good. Too bad that ended up being a bit messy.

@jhp-lanl
Copy link
Collaborator Author

jhp-lanl commented Apr 9, 2025

Okay I reversed myself and decided to just make constexpr work. Testing on the LANL CI right now

@jhp-lanl
Copy link
Collaborator Author

jhp-lanl commented Apr 9, 2025

I had some issues with the python API, but that appears to be working with some minor changes. Will merge when all tests pass (maybe with the exception of the Ampere nodes since the queue times for those are pretty long today).

@jhp-lanl jhp-lanl requested a review from Yurlungur April 9, 2025 22:39
@jhp-lanl
Copy link
Collaborator Author

jhp-lanl commented Apr 9, 2025

@Yurlungur tests pass. If you approve, feel free to merge

Copy link
Collaborator

@Yurlungur Yurlungur left a comment

Choose a reason for hiding this comment

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

LGTM

@Yurlungur Yurlungur merged commit b5a3024 into main Apr 10, 2025
9 checks passed
@Yurlungur Yurlungur deleted the jhp/nlambda branch April 10, 2025 00:18
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