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 Tensor3D from MATLAB #290

Merged
merged 21 commits into from
Jun 5, 2023
Merged

Detach Tensor3D from MATLAB #290

merged 21 commits into from
Jun 5, 2023

Conversation

willGraham01
Copy link
Collaborator

@willGraham01 willGraham01 commented May 11, 2023

Context/Description

Detaches Tensor3D class from MATLAB. This class is now robust enough to read from either standard C++ buffers or from MATLAB data of the appropriate shape. We also no longer need to use malloc for memory management.

The dependant classes still depend on MATLAB for reading/writing from the input file, which can be addressed in another pull request.

More details

Tensor3D wraps a std::vector.

  • operator() is used to index/reference the data with three indices.
  • We cannot use the operator[] since this can only take 1 argument unless we up our C++ standard to 23.
  • The operator[] can still be invoked if passed an ijk struct from cell_coordinate.h, but this simply unpacks and passes the information to operator().
  • This causes these changes to be noisy, since we have to change the call methods for all classes that have Tensor3D members or parents.

Data is stored as a strided vector

  • Row-major format is used since C and hdf5 use this convention. IE Later dimensions have faster-varying indices.
  • Using a strided vector means that zero(), frobenius(), and any other all-element operations can be carried out using std::vector methods.
  • The class stores the "3D" dimensions that it is mimicking when they are set.

Reading 3D buffers

  • Tensor3D::initialise allows us to read from ***T buffers.
  • To comply with std::vector ownership laws and avoid memory leaks, data is copied into Tensor3Ds storage rather than reassigning the underlying pointer.

File restructuring

In an attempt to curb the rampant expansion of the arrays.h file, classes and structs within this file have begun to be broken out into separate files within the arrays directory.

Testing

Unit tests for Tensor3D, DTilde, and IncidentField have been preserved, and continue to pass. Tensor3D unit tests have been updated slightly to reflect the new call syntax.

@willGraham01 willGraham01 marked this pull request as ready for review May 11, 2023 13:04
@samcunliffe samcunliffe force-pushed the wgraham-DispersiveMultiLayer-no-matlab branch from 821f1ab to d4d0cfd Compare May 12, 2023 12:40
Base automatically changed from wgraham-DispersiveMultiLayer-no-matlab to 181-hdf5-io-pairdev May 12, 2023 12:43
@samcunliffe samcunliffe self-requested a review May 12, 2023 12:44
@samcunliffe samcunliffe added technical Technical and meta issues, not related to physics but infrastructure. review required A review approval is required before merging labels May 12, 2023
@codecov
Copy link

codecov bot commented May 19, 2023

Codecov Report

❗ No coverage uploaded for pull request base (hdf5-cleaner-helper-methods@14cd103). Click here to learn what that means.
Patch has no changes to coverable lines.

Additional details and impacted files
@@                     Coverage Diff                      @@
##             hdf5-cleaner-helper-methods   #290   +/-   ##
============================================================
  Coverage                               ?    27%           
============================================================
  Files                                  ?     75           
  Lines                                  ?   3699           
  Branches                               ?      0           
============================================================
  Hits                                   ?    992           
  Misses                                 ?   2707           
  Partials                               ?      0           

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

@samcunliffe
Copy link
Member

Can this be targetted at main yet? Or do we need all of #250 's changes to go in first?

@willGraham01
Copy link
Collaborator Author

willGraham01 commented May 23, 2023

Can this be targetted at main yet? Or do we need all of #250 's changes to go in first?

Need all of #250 I'm afraid. However this, #303 <- #307, and my current attempts to deal with XYZVector (#315) are all parallel from there on out.

@willGraham01 willGraham01 changed the base branch from 181-hdf5-io-pairdev to main June 1, 2023 10:38
@willGraham01 willGraham01 changed the base branch from main to 181-cherry-pick-pr286 June 1, 2023 10:38
@willGraham01 willGraham01 changed the base branch from 181-cherry-pick-pr286 to main June 1, 2023 10:39
@willGraham01 willGraham01 changed the base branch from main to 181-cherry-pick-pr286 June 1, 2023 11:02
Base automatically changed from 181-cherry-pick-pr286 to main June 1, 2023 15:02
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.

V. minor suggestions: a bit more docs and one thing you might want to rename.

tdms/tests/unit/array_tests/test_DTilde.cpp Show resolved Hide resolved
tdms/include/arrays.h Show resolved Hide resolved
tdms/include/arrays/dtilde.h Outdated Show resolved Hide resolved
tdms/include/arrays/incident_field.h Show resolved Hide resolved
tdms/include/arrays/tensor3d.h Outdated Show resolved Hide resolved
@UCL UCL deleted a comment from samcunliffe Jun 5, 2023
willGraham01 and others added 14 commits June 5, 2023 15:10
Looks like some field components aren't even being initialised, or set, since the Fro norms all come back as 0 before seg-faulting on double free.
- Tensor3D wraps a vector<T> member rather than directly inheriting from the class
- Fixes a typo in the field tests when converting old -> new syntax for accessing Tensor3Ds
Co-authored-by: Sam Cunliffe <samcunliffe@users.noreply.github.com>
@samcunliffe samcunliffe disabled auto-merge June 5, 2023 14:16
@samcunliffe samcunliffe changed the base branch from main to hdf5-cleaner-helper-methods June 5, 2023 14:16
Base automatically changed from hdf5-cleaner-helper-methods to main June 5, 2023 14:56
@samcunliffe samcunliffe added this pull request to the merge queue Jun 5, 2023
@samcunliffe samcunliffe removed this pull request from the merge queue due to a manual request Jun 5, 2023
@samcunliffe samcunliffe merged commit 18840c0 into main Jun 5, 2023
@samcunliffe samcunliffe deleted the wgraham-Tensor3D_no_matlab branch June 5, 2023 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review required A review approval is required before merging technical Technical and meta issues, not related to physics but infrastructure.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants