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

Refactor iterator [1/N] #64

Merged
merged 2 commits into from
Sep 13, 2022
Merged

Refactor iterator [1/N] #64

merged 2 commits into from
Sep 13, 2022

Conversation

t-young31
Copy link
Member

@t-young31 t-young31 commented Aug 9, 2022

Works towards closing #63 . This PR:

  • Adds a couple of simple classes for electric E and magnetic H fields
  • Adds a Timer class suitable for printing intervals and recording the time between starts and ends. Maximises code reuse and more clearly expresses intention
  • Adds a test of the phasor_norm functions (equiv. to extractPhasorENorm and extractPhasorHNorm)
  • Adds namespace prefixes to header files removing the requirement for using namespace std before including headers
  • Starts a SimulationParameters class to hold the full set of parameters required for the simulation
  • Removes the unused matlabio_globals.h header
  • Moves numeric.h to the correct directory
  • Adds some comments for the simulation dimensionality
  • Starts to replace 0 -> false and 1 -> true, for readability
  • Implements some clang-tidy suggestions in numerical_derivative.cpp
  • Uses the built in pi constant from cmath
  • Deletes some dead code in mesh_base.cpp

With a view to have a modest diff this will be continued in other PRs


Timings

On an M1 Pro Macbook. Note that there is a few % margin of error between reruns.

Initial
$ export OMP_NUM_THREADS=1
$ time py.test ../tests/system/test_arc01.py 
real	1m57.158s
user	1m55.738s
sys	0m0.496s
Final
real	1m57.463s
user	1m56.250s
sys	0m0.565s

Dependencies

@t-young31 t-young31 linked an issue Aug 9, 2022 that may be closed by this pull request
7 tasks
@t-young31 t-young31 changed the title Refactor iterator Refactor iterator [1/N] Aug 9, 2022
tdms/src/numerical_derivative.cpp Show resolved Hide resolved
tdms/src/iterator.cpp Outdated Show resolved Hide resolved
tdms/src/magnetic_field.cpp Show resolved Hide resolved
tdms/src/electric_field.cpp Show resolved Hide resolved
@t-young31 t-young31 mentioned this pull request Aug 9, 2022
@t-young31 t-young31 marked this pull request as ready for review August 10, 2022 06:50
@t-young31 t-young31 requested a review from prmunro August 10, 2022 06:50
@t-young31 t-young31 added the review required A review approval is required before merging label Aug 10, 2022
@t-young31 t-young31 deleted the branch UCL:main August 10, 2022 11:27
@t-young31 t-young31 closed this Aug 10, 2022
@t-young31 t-young31 reopened this Aug 10, 2022
@t-young31 t-young31 changed the base branch from mg/ci-windows-macos to main August 10, 2022 12:34
@t-young31 t-young31 removed the review required A review approval is required before merging label Aug 17, 2022
@@ -0,0 +1,10 @@

class SimulationParameters{
Copy link
Member Author

Choose a reason for hiding this comment

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

this class will be expanded significantly in later PRs

tdms/src/timer.cpp Outdated Show resolved Hide resolved
tdms/tests/unit/test_fields.cpp Outdated Show resolved Hide resolved
tdms/tests/unit/test_fields.cpp Outdated Show resolved Hide resolved
tdms/include/globals.h Outdated Show resolved Hide resolved
tdms/include/field.h Outdated Show resolved Hide resolved
tdms/src/iterator.cpp Outdated Show resolved Hide resolved
tdms/src/iterator.cpp Outdated Show resolved Hide resolved
@giordano
Copy link
Member

The reason why M_PI couldn't be found on Windows is because having to define the macro _USE_MATH_DEFINES before including cmath (or math.h) introduces an ordering problem: globals.h has to be included before cmath (or math.h) or any other header file which includes them (like complex), because when it gets to globals.h the math.h header file wouldn't be loaded again a second time, and the first time the definition of the math constants was skipped because _USE_MATH_DEFINES wasn't defined.

This patch should do the trick:

diff --git a/tdms/src/electric_field.cpp b/tdms/src/electric_field.cpp
index 0e02b64..f338606 100644
--- a/tdms/src/electric_field.cpp
+++ b/tdms/src/electric_field.cpp
@@ -1,5 +1,5 @@
-#include <complex>
 #include "globals.h"
+#include <complex>
 #include "field.h"
 
 using namespace std;
diff --git a/tdms/src/iterator.cpp b/tdms/src/iterator.cpp
index 01a6aff..f47e67e 100644
--- a/tdms/src/iterator.cpp
+++ b/tdms/src/iterator.cpp
@@ -4,6 +4,7 @@
  *                 such as phasor extraction etc. Works in both pulsed 
  *                 and steady state mode.
  ******************************************************************/
+#include "globals.h"
 #include <complex>
 #include <algorithm>
 #include <cstring>
@@ -15,7 +16,6 @@
 #include "numeric.h"
 #include "mesh_base.h"
 #include "numerical_derivative.h"
-#include "globals.h"
 #include "matlabio.h"
 #include "mesh_base.h"
 #include "timer.h"
diff --git a/tdms/src/magnetic_field.cpp b/tdms/src/magnetic_field.cpp
index 3c8a7e6..6f585d4 100644
--- a/tdms/src/magnetic_field.cpp
+++ b/tdms/src/magnetic_field.cpp
@@ -1,5 +1,5 @@
-#include <complex>
 #include "globals.h"
+#include <complex>
 #include "field.h"
 
 using namespace std;
diff --git a/tdms/src/numerical_derivative.cpp b/tdms/src/numerical_derivative.cpp
index 78e888c..85df4f2 100644
--- a/tdms/src/numerical_derivative.cpp
+++ b/tdms/src/numerical_derivative.cpp
@@ -1,7 +1,7 @@
+#include "globals.h"
 #include <cmath>
 #include <cstdlib>
 #include <fftw3.h>
-#include "globals.h"
 #include "numerical_derivative.h"
 #include "matlabio.h"

@t-young31
Copy link
Member Author

thanks for investigating! I'd be rather inclined to leave the it as is, because adding in a constraint on ordering header files is a little annoying, imo

@t-young31 t-young31 added the review required A review approval is required before merging label Aug 25, 2022
Remove unused pointers


Don't throw with unused


Add pkg executable

Refactor numeric.cpp


Refactor numerical_derivative



Start iterator refactor

Add timers

Add catch2 fetch message

Don't narrow int


Doc + indentation + type decl


Delete unused pragmas on CI


Add simulation params struct remove unused globals


Extract angular norms


Add H field test


Fix time order

Use relative difference


Use built in pi constant


Use langle rangle


auto types


Include math defines


Delete unused code


Fix typo in tdms/include/globals.h

Revert pi import
Copy link
Collaborator

@prmunro prmunro left a comment

Choose a reason for hiding this comment

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

These changes all look reasonable

@t-young31 t-young31 merged commit 1f4207e into UCL:main Sep 13, 2022
@t-young31 t-young31 deleted the refactor branch September 13, 2022 14:54
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants