Kill DoubleVolumeDataLayout #267
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
yngve793
reviewed
May 30, 2024
internal/core/datahandle.cpp
Outdated
|
|
||
| std::vector<float> coordinates_a(coordinates_buffer_size); | ||
| this->m_metadata.offset_samples_to_match_cube_a(coordinates, ntraces, &coordinates_a); | ||
| std::vector<voxel> coordinates_a(ntraces); |
There was a problem hiding this comment.
This changes causes a compiler error on Mac. Can we change it back to a float array?
There was a problem hiding this comment.
Can I get more info about the mac compilation error? Maybe it could be fixed some other way?
(and maybe we need a CI for mac then, but don't know how difficult it is to set openvds there and how it would vary from Mac to Mac)
There was a problem hiding this comment.
I guess there are minor deviations between C++ compilators. Apparently some of them have extra features on top of the standard and these might deviate. We can use my machine to test variants if you want. Error below:
YVI@AC-FVFF953RQ05P:~/vds-slice (kill_doublevolumedatalayout)
└─(07:51:26, 19 files, 192b)─> slice_cmake_build
[ 3%] Building CXX object internal/core/CMakeFiles/cppcore.dir/attribute.cpp.o
[ 6%] Building CXX object internal/core/CMakeFiles/cppcore.dir/boundingbox.cpp.o
[ 10%] Building CXX object internal/core/CMakeFiles/cppcore.dir/cppapi_data.cpp.o
[ 13%] Building CXX object internal/core/CMakeFiles/cppcore.dir/cppapi_metadata.cpp.o
[ 16%] Building CXX object internal/core/CMakeFiles/cppcore.dir/datahandle.cpp.o
In file included from /Users/YVI/vds-slice/internal/core/datahandle.cpp:1:
In file included from /Users/YVI/vds-slice/internal/core/datahandle.hpp:4:
In file included from /Library/Developer/CommandLineTools/SDKs/MacOSX14.4.sdk/usr/include/c++/v1/memory:886:
/Library/Developer/CommandLineTools/SDKs/MacOSX14.4.sdk/usr/include/c++/v1/__memory/allocator.h:172:12: error: object expression of non-scalar type 'float[6]' cannot be used in a pseudo-destructor expression
__p->~_Tp();
~~~^
/Library/Developer/CommandLineTools/SDKs/MacOSX14.4.sdk/usr/include/c++/v1/__memory/allocator_traits.h:315:13: note: in instantiation of member function 'std::allocator<float[6]>::destroy' requested here
__a.destroy(__p);
^
/Library/Developer/CommandLineTools/SDKs/MacOSX14.4.sdk/usr/include/c++/v1/vector:944:25: note: in instantiation of function template specialization 'std::allocator_traits<std::allocator<float[6]>>::destroy<float[6], void>' requested here
__alloc_traits::destroy(__alloc(), std::__to_address(--__soon_to_be_end));
^
/Library/Developer/CommandLineTools/SDKs/MacOSX14.4.sdk/usr/include/c++/v1/vector:938:29: note: in instantiation of member function 'std::vector<float[6]>::__base_destruct_at_end' requested here
void __clear() _NOEXCEPT {__base_destruct_at_end(this->__begin_);}
^
/Library/Developer/CommandLineTools/SDKs/MacOSX14.4.sdk/usr/include/c++/v1/vector:489:20: note: in instantiation of member function 'std::vector<float[6]>::__clear' requested here
__vec_.__clear();
^
/Library/Developer/CommandLineTools/SDKs/MacOSX14.4.sdk/usr/include/c++/v1/__utility/exception_guard.h:86:7: note: in instantiation of member function 'std::vector<float[6]>::__destroy_vector::operator()' requested here
__rollback_();
^
/Library/Developer/CommandLineTools/SDKs/MacOSX14.4.sdk/usr/include/c++/v1/__utility/exception_guard.h:137:10: note: in instantiation of member function 'std::__exception_guard_exceptions<std::vector<float[6]>::__destroy_vector>::~__exception_guard_exceptions' requested here
return __exception_guard<_Rollback>(std::move(__rollback));
^
/Users/YVI/vds-slice/internal/core/datahandle.cpp:319:24: note: in instantiation of member function 'std::vector<float[6]>::vector' requested here
std::vector<voxel> coordinates_a(ntraces);
^
1 error generated.
make[2]: *** [internal/core/CMakeFiles/cppcore.dir/datahandle.cpp.o] Error 1
make[1]: *** [internal/core/CMakeFiles/cppcore.dir/all] Error 2
make: *** [all] Error 2
YVI@AC-FVFF953RQ05P:~/vds-slice (kill_doublevolumedatalayout)
└─(07:51:37, 19 files, 192b)─>
yngve793
reviewed
Jun 3, 2024
internal/core/datahandle.cpp
Outdated
|
|
||
| std::vector<float> coordinates_a(coordinates_buffer_size); | ||
| this->m_metadata.offset_samples_to_match_cube_a(coordinates, ntraces, &coordinates_a); | ||
| std::vector<voxel> coordinates_a(ntraces); |
There was a problem hiding this comment.
I guess there are minor deviations between C++ compilators. Apparently some of them have extra features on top of the standard and these might deviate. We can use my machine to test variants if you want. Error below:
YVI@AC-FVFF953RQ05P:~/vds-slice (kill_doublevolumedatalayout)
└─(07:51:26, 19 files, 192b)─> slice_cmake_build
[ 3%] Building CXX object internal/core/CMakeFiles/cppcore.dir/attribute.cpp.o
[ 6%] Building CXX object internal/core/CMakeFiles/cppcore.dir/boundingbox.cpp.o
[ 10%] Building CXX object internal/core/CMakeFiles/cppcore.dir/cppapi_data.cpp.o
[ 13%] Building CXX object internal/core/CMakeFiles/cppcore.dir/cppapi_metadata.cpp.o
[ 16%] Building CXX object internal/core/CMakeFiles/cppcore.dir/datahandle.cpp.o
In file included from /Users/YVI/vds-slice/internal/core/datahandle.cpp:1:
In file included from /Users/YVI/vds-slice/internal/core/datahandle.hpp:4:
In file included from /Library/Developer/CommandLineTools/SDKs/MacOSX14.4.sdk/usr/include/c++/v1/memory:886:
/Library/Developer/CommandLineTools/SDKs/MacOSX14.4.sdk/usr/include/c++/v1/__memory/allocator.h:172:12: error: object expression of non-scalar type 'float[6]' cannot be used in a pseudo-destructor expression
__p->~_Tp();
~~~^
/Library/Developer/CommandLineTools/SDKs/MacOSX14.4.sdk/usr/include/c++/v1/__memory/allocator_traits.h:315:13: note: in instantiation of member function 'std::allocator<float[6]>::destroy' requested here
__a.destroy(__p);
^
/Library/Developer/CommandLineTools/SDKs/MacOSX14.4.sdk/usr/include/c++/v1/vector:944:25: note: in instantiation of function template specialization 'std::allocator_traits<std::allocator<float[6]>>::destroy<float[6], void>' requested here
__alloc_traits::destroy(__alloc(), std::__to_address(--__soon_to_be_end));
^
/Library/Developer/CommandLineTools/SDKs/MacOSX14.4.sdk/usr/include/c++/v1/vector:938:29: note: in instantiation of member function 'std::vector<float[6]>::__base_destruct_at_end' requested here
void __clear() _NOEXCEPT {__base_destruct_at_end(this->__begin_);}
^
/Library/Developer/CommandLineTools/SDKs/MacOSX14.4.sdk/usr/include/c++/v1/vector:489:20: note: in instantiation of member function 'std::vector<float[6]>::__clear' requested here
__vec_.__clear();
^
/Library/Developer/CommandLineTools/SDKs/MacOSX14.4.sdk/usr/include/c++/v1/__utility/exception_guard.h:86:7: note: in instantiation of member function 'std::vector<float[6]>::__destroy_vector::operator()' requested here
__rollback_();
^
/Library/Developer/CommandLineTools/SDKs/MacOSX14.4.sdk/usr/include/c++/v1/__utility/exception_guard.h:137:10: note: in instantiation of member function 'std::__exception_guard_exceptions<std::vector<float[6]>::__destroy_vector>::~__exception_guard_exceptions' requested here
return __exception_guard<_Rollback>(std::move(__rollback));
^
/Users/YVI/vds-slice/internal/core/datahandle.cpp:319:24: note: in instantiation of member function 'std::vector<float[6]>::vector' requested here
std::vector<voxel> coordinates_a(ntraces);
^
1 error generated.
make[2]: *** [internal/core/CMakeFiles/cppcore.dir/datahandle.cpp.o] Error 1
make[1]: *** [internal/core/CMakeFiles/cppcore.dir/all] Error 2
make: *** [all] Error 2
YVI@AC-FVFF953RQ05P:~/vds-slice (kill_doublevolumedatalayout)
└─(07:51:37, 19 files, 192b)─>
yngve793
approved these changes
Jun 4, 2024
Order of members declaration matters during object initialization. DoubleMetadata is dependent on single metadatas. Hence correcting the order prevents weird segfaults during refactoring.
Original idea was to expose grid metadata to the user and thus bypass these calculations in tests altogether. However it was decided against directly exposing it as it would be confusing for the end user because of the inconsistencies in the domain. So calculations are changed instead to - get rid of the openvds coordinate transformer dependency - illustrate the way user might get grid information from currently exposed data
When looking for combination of files to use in tests, it is easier to directly see full axis range file represents
achaikou
force-pushed
the
kill_doublevolumedatalayout
branch
from
2223f5b
to
b5a067f
Compare
Our CoordinateTransformer is a simple wrapper over IJKCoordinateTransformer in openvds for single files and its own structure for double files.
Currently Axis is a simple wrapper over AxisDescriptor. AxisDescriptor is responsible not only for nsamples, min and max, but also for name and unit, which in openvds world are char*. It means that our Axis functionality depends on some external memory we do not control. With introduction of double metadata this becomes a problem if we try to minimize number of dependencies. So idea is to change Axis from being a wrapper over AxisDescriptor to its own structure that just uses AxisDescriptor class for calculations.
In preparation to remove DoubleVolumeDataLayout, do sanity checks from its constructor in DoubleMetadata one. Notes: - Comparison between dimension maps is dropped as it is actually checked through names comparison for every dimension when creating double axes. Internally openvds also creates dimension map by matching dimensions with expected axes names. - unitStep comparisons are exchanged with comparison of Inline and Crossline spacing. As we support axis names to be only of annotation kind (not XYZ or IJK), openvds seems to calculate steps based on spacing/origin only. So comparing source information instead of calculated versions should be enough. - Origins are compared directly, without epsilon. For now the check is removed to get information on whether origin data would be exactly the same in the source files, would be inside the 0.0001 margin, outside of it or should not be absolute value at all. The check should be back once we gather information. - DoubleMetadataClass is made a friend of SingleMetadataClass as anyway Double operates on Singles for its operations. Alternative option to expose required function through public functions would also be acceptable, but friends solution won due to simplicity and already tight logical coupling between classes.
It makes sense to handle transformation of coordinates between intersection cube and cubes a/b with coordinate transformer.
GetDimensionName and GetDimensionality are exchanged with references to one of the metadatas, just as in the DoubleVolumeDataLayout. Correspondence of names and dimensions is verified in constructor, so even if these methods are called before constructor verification is complete, verification will fail later anyway.
Class depends too much on the openvds internals as it needs to implement openvds interface. It is viable way to deal with the problem, but removing as we can as well leave without it.
achaikou
force-pushed
the
kill_doublevolumedatalayout
branch
from
b5a067f
to
8a5a2fb
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR aims to remove DoubleVolumeDataLayout as I believe we should try avoid inheriting from openvds classes where possible. I think inheriting from their classes was not intended by openvds, thus it might cause troubles in future during updates.