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

Avoid raw pointers in projecting implementation #85

Merged
merged 2 commits into from
Apr 11, 2024

Conversation

lisitsyn
Copy link
Owner

Probably fixes #84

@lisitsyn
Copy link
Owner Author

hey @iglesias would you like to take a look? thanks

@iglesias
Copy link
Collaborator

Hey! I run and it fixed 2 of the errors. There's still this one (summarized):

2: ==191523==ERROR: AddressSanitizer: requested allocation size 0x2d79883d20010 (0x2d79883d21010 after adjustments for alignment, red zones etc.) exceeds maximum supported size of 0x10000000000 (thread T0)
[...]
2:     #7 0x651ab7876242 in void Eigen::PlainObjectBase<Eigen::Matrix<double, -1, -1, 0, -1, -1> >::_init2<int, int>(long, long, Eigen::internal::enable_if<true, int>::type*) /usr/include/eigen3/Eigen/src/Core/PlainObjectBase.h:810
2:     #8 0x651ab7837f9f in Eigen::Matrix<double, -1, -1, 0, -1, -1>::Matrix<int, int>(int const&, int const&) /usr/include/eigen3/Eigen/src/Core/Matrix.h:340
2:     #9 0x651ab79b0321 in Eigen::Matrix<double, -1, -1, 0, -1, -1> tapkee::tapkee_internal::compute_distance_matrix<__gnu_cxx::__normal_iterator<float*, std::vector<float, std::allocator<float> > >, float_distance_callback>(__gnu_cxx::__normal_iterator<float*, std::vector<float, std::allocator<float> > >, __gnu_cxx::__normal_iterator<float*, std::vector<float, std::allocator<float> > >, float_distance_callback) tapkee/include/tapkee/routines/multidimensional_scaling.hpp:114
2:     #10 0x651ab7917b97 in tapkee::tapkee_internal::ImplementationBase<__gnu_cxx::__normal_iterator<float*, std::vector<float, std::allocator<float> > >, tapkee::dummy_kernel_callback<float>, float_distance_callback, tapkee::dummy_features_callback<float> >::embedMultidimensionalScaling() tapkee/include/tapkee/methods.hpp:195
2:     #11 0x651ab78a95bb in tapkee::tapkee_internal::ImplementationBase<__gnu_cxx::__normal_iterator<float*, std::vector<float, std::allocator<float> > >, tapkee::dummy_kernel_callback<float>, float_distance_callback, tapkee::dummy_features_callback<float> >::embedUsing(tapkee::DimensionReductionMethod) tapkee/include/tapkee/methods.hpp:104
2:     #12 0x651ab78643d2 in tapkee::TapkeeOutput tapkee::embed<__gnu_cxx::__normal_iterator<float*, std::vector<float, std::allocator<float> > >, tapkee::dummy_kernel_callback<float>, float_distance_callback, tapkee::dummy_features_callback<float> >(__gnu_cxx::__normal_iterator<float*, std::vector<float, std::allocator<float> > >, __gnu_cxx::__normal_iterator<float*, std::vector<float, std::allocator<float> > >, tapkee::dummy_kernel_callback<float>, float_distance_callback, tapkee::dummy_features_callback<float>, stichwort::ParametersSet) tapkee/include/tapkee/embed.hpp:117
[...]
2: ==191523==HINT: if you don't care about these errors you may set allocator_may_return_null=1
2: SUMMARY: AddressSanitizer: allocation-size-too-big /usr/src/debug/gcc/gcc/libsanitizer/asan/asan_malloc_linux.cpp:69 in __interceptor_malloc

I doubt it is relevant though. It passes without the sanitizer and there's no kind of out of memory error. And the name of the test fits with it having this type of error ':-D

tapkee.log

ProjectingFunction(ProjectionImplementation* impl) : implementation(impl)
{
}
ProjectingFunction(const ProjectingFunction& other) : implementation(other.implementation)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the copy constructor explicitly needed?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yes, right. Going to remove that.

ProjectingFunction(ProjectionImplementation* impl) : implementation(impl){};
//! Destroys current implementation
void clear()
ProjectingFunction() : implementation(NULL)
Copy link
Collaborator

Choose a reason for hiding this comment

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

A minor one, but let's use nullptr instead?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Replacing..

@iglesias
Copy link
Collaborator

lgtm 🚢

@iglesias iglesias merged commit f1a361a into main Apr 11, 2024
4 of 6 checks passed
@lisitsyn lisitsyn deleted the fix/shared-projecting-impl branch April 12, 2024 09:38
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.

Look into attached unit test logs
2 participants