Skip to content

Fh kokkos b spline - #3

Open
hsiehhRPI wants to merge 97 commits into
mainfrom
fh_Kokkos_BSpline
Open

Fh kokkos b spline#3
hsiehhRPI wants to merge 97 commits into
mainfrom
fh_Kokkos_BSpline

Conversation

@hsiehhRPI

Copy link
Copy Markdown
Collaborator

This branch included Kokkos version of the BSpline inplementation. The main file is BSplineKokkos2D.h which contains Kokkos version of the BSpline that utilize 2D view for control points. 1st and 2nd derivative evaluation are implemented.

Hsieh and others added 30 commits March 8, 2026 12:37
Comment thread BSplineKokkos.cpp Outdated
Comment thread BSplineKokkos.h Outdated
Comment thread BSplineKokkos.h Outdated
Comment thread BSplineKokkos2D.cpp Outdated
Comment thread BSplineKokkos2D.h
Comment thread testKokkos1stDerivative.cpp Outdated
Comment thread testKokkos1stDerivative.cpp Outdated
Comment thread BSplineKokkos.h
Comment thread testKokkos1stDerivative.cpp Outdated
Comment thread testSplineKokkosBase.cpp Outdated
@cwsmith

cwsmith commented May 11, 2026

Copy link
Copy Markdown
Contributor

@hsiehhRPI Was clang-format ran on this? I saw some long lines (>80 chars) that I would have expected it to break into multiple lines.

@cwsmith cwsmith left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please carefully review the comments on the rank 1 implementation/tests/etc and ensure that they are addressed in the rank 2 version if the same code exists there.

Hsieh added 21 commits June 5, 2026 03:31
…tter: Hsieh <hsiehh@checkers.scorec.rpi.edu>
…itter: Hsieh <hsiehh@checkers.scorec.rpi.edu>
… test for GPU# Committer: Hsieh <hsiehh@checkers.scorec.rpi.edu>
…itter: Hsieh <hsiehh@checkers.scorec.rpi.edu>
…h cpu and gpu# Committer: Hsieh <hsiehh@checkers.scorec.rpi.edu>
…mespace# Committer: Hsieh <hsiehh@checkers.scorec.rpi.edu>
…views in derivative call functions Committer: Hsieh <hsiehh@checkers.scorec.rpi.edu>
…tion to simplify the input to the device functions # Committer: Hsieh <hsiehh@checkers.scorec.rpi.edu>
…s # Committer: Hsieh <hsiehh@checkers.scorec.rpi.edu>
@hsiehhRPI

Copy link
Copy Markdown
Collaborator Author

Hello Dr. Smith, I have ran clang-format and have a commit with only clang formatting and no logical changes. The line lengths should now be at most 80 characters.

Comment thread BSplineKokkos2D.h
const int order, const Kokkos::View<double *, MemSpace> knots,
const Kokkos::View<double *[2], MemSpace> ctrlPts_1stD) const {
// DeBoor's algorithm for BSpline 1st deriv calculation
const int MAX_DEGREE = 3;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this could be useful outside this function - where else is this hardcoded?

@hsiehhRPI hsiehhRPI Aug 8, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This was in both Kokkos De Boor's Device functions. I will move them outside.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

When I move it outside of the function, should it be declared as a static constexpr inside the class or as a member variable?

Comment thread BSplineKokkos2D.h
const int MAX_DEGREE = 3;
int lKnot = order;
lKnot--;
int resultOrder = lKnot;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

int resultOrder = order -1; would be much easier to read

Comment thread BSplineKokkos2D.h
int leftPt = 0;

while (knots(lKnot+1) < x) {
// double x = xVals(offset);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

please remove code that is commented out

Comment thread BSplineKokkos2D.h
}

int order_t = lKnot-1;
const int MAX_DEGREE = 3;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

move this definition to a place both functions can access it - defining constants in multiple locations is likely to cause problems later (i.e., someone changes one but forgets/doesn't know to change the other)

Comment thread testKokkos2D1stDeriv.cpp Outdated
#include <numeric>
#include <string>

double EPSILON = 1e-12;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please make this const and move it to where it is used within the function.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This has been resolved.

Comment thread testKokkos2D2ndDeriv.cpp Outdated
#include <numeric>
#include <string>

double EPSILON = 1e-12;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

see comment on other test

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This has been resolved.

Comment thread testSplineKokkos2DBase.cpp Outdated
// For checking if the content of the splines are correct
//

double EPSILON = 1e-12;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

same comment as other tests

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This has been resolved.

Comment thread testSplineKokkos2DBase.cpp Outdated
Comment on lines 62 to 63
auto intView = Kokkos::create_mirror_view(kokkosBSP.getOrder());
Kokkos::deep_copy(intView, kokkosBSP.getOrder());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

use the kokkos function create_mirror_view_and_copy if you just want a host copy of the 'order' device array

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This has been resolved.

Comment thread testSplineKokkos2DBase.cpp Outdated
Comment on lines 73 to 74
auto double2DView = Kokkos::create_mirror_view(kokkosBSP.getCtrlPts());
Kokkos::deep_copy(double2DView, kokkosBSP.getCtrlPts());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

same as above

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This has been resolved.

Comment thread testSplineKokkos2DBase.cpp Outdated
Comment on lines 93 to 94
auto doubleView = Kokkos::create_mirror_view(kokkosBSP.getKnots());
Kokkos::deep_copy(doubleView, kokkosBSP.getKnots());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

same as above

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This has been resolved.

Comment thread testSplineKokkos2DBase.cpp Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

same as above

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This has been resolved.

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