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.

Sign up for GitHub

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

Jump to bottom

Add functions to check if the provided interpolation points are valid #686

Merged
merged 10 commits into from
Dec 4, 2024
Merged

Add functions to check if the provided interpolation points are valid #686

merged 10 commits into from
Dec 4, 2024

Conversation

EmilyBourne
Copy link
Collaborator

Closes #659

tpadioleau
tpadioleau reviewed Nov 26, 2024
View reviewed changes
include/ddc/kernels/splines/spline_builder.hpp Outdated
auto current_cell_end_idx = ddc::discrete_space<BSplines>().break_point_domain().front() + 1;
ddc::for_each(interpolation_domain(), [&](auto idx) {
ddc::Coordinate<continuous_dimension_type> point = ddc::coordinate(idx);
if (point == ddc::coordinate(current_cell_end_idx)) {
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we relax this condition with a tolerance to avoid numerical instabilities ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No I don't think so. The less than and greater than cases are both covered. Equal to only needs treating separately because it is in 2 cells. If we are at knot+eps then there is no ambiguity so no special treatment is necessary

Copy link
Member

Choose a reason for hiding this comment

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

I see, so if I understand correctly, in this case you assign the point to one cell ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes exactly. I can add a comment to explain this

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Comments added : 3e3d35c

include/ddc/kernels/splines/spline_builder.hpp
Copy link
Member

Choose a reason for hiding this comment

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

Sorry but I don't get the logic of the check. Are we checking the number of points in each cell ? If so shouldn't current_cell_end_idx increment in the loop as soon as point > ddc::coordinate(current_cell_end_idx) is true ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm, I agree. Either I missed something while copy/pasting or the error output isn't correct in Gysela either. I don't see where current_cell_idx is updated

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed 26840d5

@EmilyBourne
Copy link
Collaborator Author

@tpadioleau Any idea what is wrong with the docs? There is nothing in the CI. If I try locally I get an error on a file that I didn't touch so maybe I have a different doxygen version

@tpadioleau
Copy link
Member

@tpadioleau Any idea what is wrong with the docs? There is nothing in the CI. If I try locally I get an error on a file that I didn't touch so maybe I have a different doxygen version

I will have a look. Sorry i thought i had already made the CI print the doxygen.log file. I had this issue not so long ago.

@EmilyBourne
Copy link
Collaborator Author

@tpadioleau Any idea what is wrong with the docs? There is nothing in the CI. If I try locally I get an error on a file that I didn't touch so maybe I have a different doxygen version

I will have a look. Sorry i thought i had already made the CI print the doxygen.log file. I had this issue not so long ago.

I think the set -xe that ensures an error is raised is causing failure before the output is printed

@tpadioleau
Copy link
Member

I think the set -xe that ensures an error is raised is causing failure before the output is printed

I think you are correct, I am trying something

tpadioleau
tpadioleau reviewed Nov 28, 2024
View reviewed changes
include/ddc/kernels/splines/bsplines_uniform.hpp Outdated
KOKKOS_INLINE_FUNCTION discrete_element_type eval_basis(
DSpan1D values,
ddc::Coordinate<CDim> const& x,
[[maybe_unused]] std::size_t degree) const;
Copy link
Member

Choose a reason for hiding this comment

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

I suspect it comes from the [[maybe_unused]] here but first I would like to see if the CI can display the error.

@tpadioleau
Copy link
Member

It worked, see https://github.com/CExA-project/ddc/actions/runs/12067535482/job/33650784704. In the end it is the same issue as the tests workflow.

@tpadioleau
Copy link
Member

Shall we call the function in the constructor ?

@tpadioleau tpadioleau force-pushed the 659-check-valid-interpolation-points branch 3 times, most recently from 6e1e38a to 240dbf1 Compare November 28, 2024 17:45
@tpadioleau
Copy link
Member

I am trying to implement tests that would trigger one of the exceptions using the SplineBuilder constructor.

I first tried with a too short interpolation_domain but it failed in compute_offset before check_valid_grid. There is an out-of-bound access inside compute_offset. This kind of break the purpose of check_valid_grid that checks the size of the interpolation_domain.

I don't have yet an idea to workaround this.

@EmilyBourne
Copy link
Collaborator Author

I am trying to implement tests that would trigger one of the exceptions using the SplineBuilder constructor.

I first tried with a too short interpolation_domain but it failed in compute_offset before check_valid_grid. There is an out-of-bound access inside compute_offset. This kind of break the purpose of check_valid_grid that checks the size of the interpolation_domain.

I don't have yet an idea to workaround this.

m_offset is not const. So can't we just initialise this value in the body of the constructor after the call to check_valid_grid?

@tpadioleau
Copy link
Member

m_offset is not const. So can't we just initialise this value in the body of the constructor after the call to check_valid_grid?

We can, the linter will complain but I can disable it.

@tpadioleau tpadioleau force-pushed the 659-check-valid-interpolation-points branch 4 times, most recently from 268938c to fdc6c0b Compare December 2, 2024 18:00
@tpadioleau tpadioleau changed the base branch from main to improve-named-constructors-non_uniform_point_sampling December 2, 2024 18:00
Base automatically changed from improve-named-constructors-non_uniform_point_sampling to main December 2, 2024 19:54
@tpadioleau tpadioleau force-pushed the 659-check-valid-interpolation-points branch 3 times, most recently from b0ef58f to 012fbb4 Compare December 2, 2024 20:10
@tpadioleau
Copy link
Member

@EmilyBourne can you review the test ?

@tpadioleau tpadioleau force-pushed the 659-check-valid-interpolation-points branch from 012fbb4 to 0452340 Compare December 3, 2024 17:15
@tpadioleau tpadioleau merged commit c02adf7 into main Dec 4, 2024
55 checks passed
@tpadioleau tpadioleau deleted the 659-check-valid-interpolation-points branch December 4, 2024 06:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tpadioleau tpadioleau tpadioleau left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

User-defined spline interpolation points
2 participants
@EmilyBourne @tpadioleau