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

Added Fastor as a full backend and frontend. #78

Merged
merged 51 commits into from Mar 15, 2023

Conversation

wermos
Copy link
Contributor

@wermos wermos commented Oct 31, 2022

The full thing is still a WIP, but I am opening a PR with the vector functions to get some feedback.

@wermos
Copy link
Contributor Author

wermos commented Oct 31, 2022

Inside math/eigen/include/algebra/math/impl/eigen_vector.hpp, there are functions defined like

/** Dot product between two input vectors
 *
 * @tparam derived_type_lhs is the first matrix (expression) template
 * @tparam derived_type_rhs is the second matrix (expression) template
 *
 * @param a the first input vector
 * @param b the second input vector
 *
 * @return the scalar dot product value
 **/
template <typename derived_type_lhs, typename derived_type_rhs>
ALGEBRA_HOST_DEVICE inline auto dot(
    const Eigen::MatrixBase<derived_type_lhs> &a,
    const Eigen::MatrixBase<derived_type_rhs> &b) {
  return a.dot(b);
}

i.e., the functions are defined in terms of Eigen::MatrixBase because the library utilizes expression templates.

Fastor also utilizes expression templates, and hence has similar Expression classes. However, they are not officially documented. Are we fine with using non-documented parts of the library? Or should the definitions of stay as they are?

@niermann999 niermann999 self-requested a review October 31, 2022 16:15
Copy link
Contributor

@niermann999 niermann999 left a comment

Choose a reason for hiding this comment

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

Looks good so far

@niermann999
Copy link
Contributor

For the formatting, you can use clang format 10.0.0 or look in the github actions tab for a downloadable artifact that contains the correctly formatted changes

@wermos
Copy link
Contributor Author

wermos commented Oct 31, 2022

For the formatting, you can use clang format 10.0.0 or look in the github actions tab for a downloadable artifact that contains the correctly formatted changes.

Sure thing. I'm not too worried about the formatting right now, because most of the changes it wants me to make are in currently empty files (which, of course, won't be empty anymore by the time I'm done with them).

@wermos
Copy link
Contributor Author

wermos commented Oct 31, 2022

However, if you want to merge this PR to the main line of work, I can fix the formatting issues. Otherwise, we can just wait until I have it all done.

@wermos
Copy link
Contributor Author

wermos commented Nov 1, 2022

I'm marking this PR as a draft until I am done so that the CI doesn't keep running for every commit I make.

@wermos wermos marked this pull request as draft November 1, 2022 05:03
@wermos wermos force-pushed the integrate-fastor branch 4 times, most recently from e1b6867 to d7c5c73 Compare November 4, 2022 09:43
@wermos wermos marked this pull request as ready for review November 4, 2022 10:12
@wermos wermos changed the title Added vector functions in terms of Fastor functions Added Fastor as a full backend and frontend. Nov 4, 2022
@wermos
Copy link
Contributor Author

wermos commented Nov 4, 2022

I think I'm done with writing the backend and frontend now; it's ready for review.

@stephenswat
Copy link
Member

Please remove the swap file from the pull request. You may want to add .swp files to your global git ignore list if this is a recurring issue for you. 🙂

@wermos
Copy link
Contributor Author

wermos commented Nov 4, 2022

Oops! I've removed the file and added *.swp to my .git/info/exclude 😅

@niermann999
Copy link
Contributor

However, if you want to merge this PR to the main line of work, I can fix the formatting issues. Otherwise, we can just wait until I have it all done.

We usually merge into main, yes.

@niermann999
Copy link
Contributor

Oops! I've removed the file and added *.swp to my .git/info/exclude sweat_smile

I still see a file storage/fastor/include/algebra/storage/.eigen.hpp.swp in the change set?

Copy link
Contributor

@niermann999 niermann999 left a comment

Choose a reason for hiding this comment

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

Cool, very nice progress!

math/fastor/include/algebra/math/impl/fastor_vector.hpp Outdated Show resolved Hide resolved
math/fastor/include/algebra/math/impl/fastor_getter.hpp Outdated Show resolved Hide resolved
math/fastor/include/algebra/math/impl/fastor_getter.hpp Outdated Show resolved Hide resolved
math/fastor/include/algebra/math/impl/fastor_getter.hpp Outdated Show resolved Hide resolved
The default behavior of Fastor, which is to have runtime assertions enabled for
debug mode, and have them disabled at runtime, is good enough. Having that
definition simply hurts the debuggability of the library. For this reason, I
decided to remove that line.
@wermos wermos force-pushed the integrate-fastor branch 2 times, most recently from 2c09edd to 983b51e Compare March 8, 2023 10:01
@wermos
Copy link
Contributor Author

wermos commented Mar 8, 2023

I finally managed to pass the CI 🥳 @niermann999 It's ready for review now.

@niermann999 niermann999 self-requested a review March 9, 2023 15:33
@niermann999 niermann999 merged commit c4e5aa4 into acts-project:main Mar 15, 2023
@wermos wermos deleted the integrate-fastor branch March 28, 2023 20: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.

None yet

3 participants