-
Notifications
You must be signed in to change notification settings - Fork 4
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
Vertex Phasors Class #209
Merged
Merged
Vertex Phasors Class #209
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
samcunliffe
approved these changes
Jan 13, 2023
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
My only really comment: I wonder if we should have an abstract Phasors
class. Can you factor out any common functionality? Does it buy us anything?
Co-authored-by: Sam Cunliffe <[email protected]>
The test failure issue is on setup-matlab's end, not ours. This reverts commit a753c26.
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.
Continues #63 |
VertexPhasors
classAlong similar lines to #205, breaks out extraction of the phasors at the user-specified vertices into its own class. This reduces the number of auxiliary functions defined in
iterator.cpp
, moving the relevant methods into this class and adapting the syntax accordingly.The
ComplexAmplitudeSample
class previously that previously handled the data storage has been absorbed into theVertexPhasors
class, and hence no longer appears in the codebase. Previously, this class was one of those inarrays.h
for data storage only.Fieldsample
class-breakoutBreaks out
FieldSample
class into it's own source and header file, since it can now handle the "fieldsample extraction" portion ofiterator.cpp
internally, within it's own class.Reason for the file-breakout:
arrays.{h,cpp}
should contain only basic data containers and related methods. Objects with their own header/source are those that are interacting with other fields, classes etc that we have defined infields.h
orarrays.h
.arrays.{h,cpp}
is a huge file of declarations and definitions, and adding complex functionality in admist all that is going to come back and bite us if we keep going like this. Might create some more subfolders insrc
if we end up breaking out a lot of files like this.Other minor things in this PR
test_FieldSample.cpp
has been moved out of thearray_tests
testing subfolder since it's no longer part ofarrays.h
.test_ComplexAmplitudeSample.cpp
has been repurposed intotest_VertexPhasors.cpp
.test_interpolation_determination.cpp
was missing aspdlog::
string.