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

Iterator refactor [3/N] #204

Merged
merged 14 commits into from
Dec 21, 2022
Merged

Iterator refactor [3/N] #204

merged 14 commits into from
Dec 21, 2022

Conversation

willGraham01
Copy link
Collaborator

@willGraham01 willGraham01 commented Dec 16, 2022

Attempting to do some small refactors to iterator.cpp that don't touch anything that will need to be touched by #70. Docstrings are added to all refactored functions, classes, etc.

List of replacements:

checkPhasorConvergence

Replaced with the Field class method max_pointwise_difference_over_max_element. The name of the old function is misleading, it is only computing an error metric and NOT checking convergence - this happens later in the iterator.cpp main loop.

Pulling this out into a class method to make things a bit cleaner. Also, both the inputs are Fields, and the function is asymmetric, so it makes sense to have this as a class method.

setGridLabels

Changed to the initialise_from method of the GridLabels class.

is_dispersive_ml

Changed to is_dispersive method of DispersiveMultilayer class.

is_conductive

Superceeded by the all_elements_less_than method of the XYZVector class. The check for whether a material is conductive is just whether at least one entry of an XYZVector exceeds a particular threshold.

@willGraham01 willGraham01 marked this pull request as ready for review December 19, 2022 15:38
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.

👍

tdms/include/field.h Outdated Show resolved Hide resolved
* @return true Background is dispersive
* @return false Background is not dispersive
*/
bool is_dispersive(int K_tot, double near_zero_tolerance=1e-15);
Copy link
Member

Choose a reason for hiding this comment

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

So we can have a non-dispersive dispersive multilayer? (DispersiveMultiLayer::is_dispersive == false)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Apparently so - there's a corresponding call in iterator.cpp. I don't have the same feel for everything in iterator.cpp yet like Tom did, so I'm guessing its an oddity for us to be in this case rather than common.

There's also another is_dispersive function for the background material (which doesn't have its own class) too.

tdms/include/field.h Outdated Show resolved Hide resolved
tdms/include/field.h Outdated Show resolved Hide resolved
tdms/include/grid_labels.h Show resolved Hide resolved
tdms/src/arrays.cpp Outdated Show resolved Hide resolved
tdms/src/iterator.cpp Outdated Show resolved Hide resolved
tdms/src/iterator.cpp Outdated Show resolved Hide resolved
tdms/src/iterator.cpp Show resolved Hide resolved
@willGraham01 willGraham01 merged commit 996129e into main Dec 21, 2022
@willGraham01 willGraham01 deleted the wgraham-refactor_iterator_3 branch December 21, 2022 09:04
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.

None yet

2 participants