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 the EHVec class from Matrix #303

Merged
merged 5 commits into from
Jun 5, 2023

Conversation

willGraham01
Copy link
Collaborator

@willGraham01 willGraham01 commented May 18, 2023

Description

The EHVec class is currently a public Matrix<fftw_complex>. It is desirable to convert Matrix<> to used strided vector storage and remove any MATLAB dependencies.

The former doesn't work for EHVec since the fftw_complex type is a double[2], which throws an error when used in templated std::vector<T> functions. Additionally, EHVec is used in such a way that fftw needs a continuous buffer to perform the forward/backward transform, which a strided vector does not guarantee.

As such, it is necessary to provide EHVec with it's own (MATLAB-free) class. This enables Matrix and its other derivatives to be freed of MATLAB influence.

Testing

  • Existing unit tests for EHVec continue to pass after the refactor.
  • Removed the EHVecTest class since it is no longer a member of the arrays.h file, and it only had a single unit test in the first place (so a class was overkill).

@codecov
Copy link

codecov bot commented May 19, 2023

Codecov Report

Patch coverage: 70% and no project coverage change.

Comparison is base (fdbfc93) 26% compared to head (4154d3d) 26%.

Additional details and impacted files
@@         Coverage Diff         @@
##           main   #303   +/-   ##
===================================
  Coverage    26%    26%           
===================================
  Files        69     71    +2     
  Lines      3674   3688   +14     
===================================
+ Hits        952    957    +5     
- Misses     2722   2731    +9     
Impacted Files Coverage Δ
tdms/include/arrays.h 29% <ø> (-3%) ⬇️
tdms/include/arrays/eh_vec.h 0% <0%> (ø)
tdms/include/field.h 0% <ø> (ø)
...ms/include/simulation_manager/simulation_manager.h 0% <ø> (ø)
tdms/src/arrays.cpp 91% <ø> (-<1%) ⬇️
tdms/src/arrays/eh_vec.cpp 88% <88%> (ø)

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

@willGraham01 willGraham01 force-pushed the wgraham-EHVec-detach-from-matrix branch from a4bc6a4 to 4e4946a Compare May 19, 2023 10:01
@willGraham01 willGraham01 force-pushed the wgraham-EHVec-detach-from-matrix branch from 4e4946a to 15b429b Compare June 1, 2023 10:42
@willGraham01 willGraham01 changed the base branch from 181-hdf5-io-pairdev to main June 1, 2023 10:42
@willGraham01 willGraham01 changed the base branch from main to 181-cherry-pick-pr286 June 1, 2023 11:03
Base automatically changed from 181-cherry-pick-pr286 to main June 1, 2023 15:02
@willGraham01 willGraham01 force-pushed the wgraham-EHVec-detach-from-matrix branch from 15b429b to 4154d3d Compare June 2, 2023 08:01
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.

Totally fine with this... but I'm a bit sad that I can't have a Matrix<std::complex>.

What was the error?

@willGraham01
Copy link
Collaborator Author

Totally fine with this... but I'm a bit sad that I can't have a Matrix<std::complex>.

What was the error?

Matrix<std::complex> doesn't complain - it's Matrix<fftw_complex> which does. This is because fftw_complex is a typedef of double[2], and then some obscure error that I think boils down to the fact that an fftw_complex is a double* so can't be used in the template.

@samcunliffe samcunliffe added this pull request to the merge queue Jun 5, 2023
Merged via the queue into main with commit f673023 Jun 5, 2023
@samcunliffe samcunliffe deleted the wgraham-EHVec-detach-from-matrix branch June 5, 2023 11:55
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