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

Detach Vectors from MATLAB #328

Merged
merged 13 commits into from
Jun 6, 2023
Merged

Detach Vectors from MATLAB #328

merged 13 commits into from
Jun 6, 2023

Conversation

willGraham01
Copy link
Collaborator

@willGraham01 willGraham01 commented Jun 2, 2023

Description

Detaches the FieldComponentsVector and FrequencyExtractVector classes from the Vector class, which depended on MATLAB.

  • These two classes have been reduced to typedefs of std::vector.

The methods these classes had are now functions inside a (new) namespace tdms_vector_utils within utils.h.

  • A lot of our existing classes have custom methods that perform identical tasks to those that these functions perform. Running a pass over our classes and refactoring those methods might give us a better idea of what should be a "class/struct" for us and what can just be typedefed.

Testing

  • FieldComponentsVector and FrequencyExtractVector no longer have their own specific tests since they are now typedefs
  • test_utils.h has additional tests for the new namespace and methods
  • HDF5Reader::read() has another test for the new overload of read, for FrequencyExtractVectors.

The scripts that produce the input data have also been updated to include additional members that are needed for these unit tests.

Blocked by:

  • Windows build does not support uint parallel for loop variables 😠

@willGraham01 willGraham01 force-pushed the wgraham-vector-refactor branch 2 times, most recently from 024fbf7 to beb960b Compare June 2, 2023 16:10
@willGraham01 willGraham01 changed the base branch from main to hdf5-identify-empty-objects June 2, 2023 16:10
@codecov
Copy link

codecov bot commented Jun 2, 2023

Codecov Report

Patch coverage: 52% and no project coverage change.

Comparison is base (c69f7f5) 25% compared to head (a3aab1c) 26%.

Additional details and impacted files
@@         Coverage Diff         @@
##           main   #328   +/-   ##
===================================
  Coverage    25%    26%           
===================================
  Files        80     81    +1     
  Lines      3660   3671   +11     
===================================
+ Hits        933    937    +4     
- Misses     2727   2734    +7     
Impacted Files Coverage Δ
tdms/include/arrays.h 45% <ø> (+1%) ⬆️
tdms/include/hdf5_io/hdf5_dimension.h 0% <ø> (ø)
tdms/include/hdf5_io/hdf5_reader.h 0% <ø> (ø)
...s/include/simulation_manager/objects_from_infile.h 0% <ø> (ø)
tdms/include/utils.h 0% <0%> (ø)
tdms/include/vertex_phasors.h 0% <ø> (ø)
tdms/src/arrays.cpp 100% <ø> (+10%) ⬆️
...mulation_manager/execute_detector_subfunctions.cpp 0% <ø> (ø)
...simulation_manager/execute_phasor_subfunctions.cpp 0% <0%> (ø)
...dms/src/simulation_manager/objects_from_infile.cpp 0% <0%> (ø)
... and 6 more

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Base automatically changed from hdf5-identify-empty-objects to main June 5, 2023 13:43
@samcunliffe
Copy link
Member

samcunliffe commented Jun 5, 2023

Windows build does not support uint parallel for loop variables 😠

sigh. Might be able to isolate the Windows problem with a macro like #if OpenMP_VERSION < 3. Working over on my fork to see if we can get it working.

#339

@willGraham01 willGraham01 marked this pull request as ready for review June 6, 2023 09:06
Copy link
Member

@samcunliffe samcunliffe left a comment

Choose a reason for hiding this comment

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

I like this very much.

(Except for the coverage weirdness that we discussed IRL. I do not like that.)

tdms/include/hdf5_io/hdf5_reader.h Outdated Show resolved Hide resolved
tdms/include/simulation_manager/objects_from_infile.h Outdated Show resolved Hide resolved
tdms/include/utils.h Show resolved Hide resolved
@willGraham01 willGraham01 mentioned this pull request Jun 6, 2023
41 tasks
@samcunliffe
Copy link
Member

🚀

@samcunliffe samcunliffe added this pull request to the merge queue Jun 6, 2023
Merged via the queue into main with commit 6ddec2d Jun 6, 2023
@samcunliffe samcunliffe deleted the wgraham-vector-refactor branch June 6, 2023 14:54
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