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

PRHS into a readable class #206

Merged
merged 11 commits into from
Dec 22, 2022
Merged

PRHS into a readable class #206

merged 11 commits into from
Dec 22, 2022

Conversation

willGraham01
Copy link
Collaborator

@willGraham01 willGraham01 commented Dec 20, 2022

Moving towards #63 |

The argument prhs to execute_simulation is rather unintuitive, being a hangover from when mexfunction was used. With regards to moving towards #70, and making the codebase a bit more readable, this object is broken out into a new class InputMatrices. This class steals some of the functionality of openandorder, and is designed to make the "unpacking" part of iterator.cpp more readable.

In theory, we can also do away with the intermediate variables that are defined in iterator.cpp and work directly with prhs now (although that is not recommended for readability purposes).

  • Constant increments of an input_counter++ are no longer needed when unpacking the input matrices - they can be accessed by passing in their names.
  • Validation that the input arrays are of the correct type/shape are performed when they are read in by the InputMatrices class, rather than at the start of execute_simulation. This avoids potentially redundant setup/memory allocation.
  • The constants like omega, Nt, etc stored in SimulationParameters are now set by calling the unpack_from_input_matrices method, rather than being done directly inside execute_simulation, reducing the code bloat.
  • A number of fprintfs -> spdlog::info in line with Use the new logging #87.
  • General Docstring tidy up in files that are touched by this PR.

@willGraham01 willGraham01 marked this pull request as ready for review December 21, 2022 12:08
extern int matricestosave_all[NOUTMATRICES_WRITE_ALL];
// indices of the matrices to save when saving a compressed output
extern int matricestosave[NOUTMATRICES_WRITE];
}
Copy link
Member

Choose a reason for hiding this comment

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

Ah, this is likely going to clash with my refactoring/reworking. (You weren't to know because I didn't tell you or push to my draft PR).

I went for std::vector<std::string> (which could be defined as const and live in a namespace). My reasoning: a vector knows its own size_t and one can do much nicer-looking loops like:

for (auto matrixname : matrixnames) {
     file.read(matrixname);
}

Copy link
Member

Choose a reason for hiding this comment

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

Come to think of it, the matrices in our input and output files define our file type. So perhaps a tdms::File is a HDF5 of this format.

tdms/include/simulation_parameters.h Show resolved Hide resolved
tdms/src/input_matrices.cpp Show resolved Hide resolved
tdms/src/input_output_names.cpp Outdated Show resolved Hide resolved
Co-authored-by: Sam Cunliffe <samcunliffe@users.noreply.github.com>
@samcunliffe
Copy link
Member

Looks all good!
When you're happy to merge, could you reject #207 at the same time, please?

@willGraham01 willGraham01 merged commit 4800bac into main Dec 22, 2022
@willGraham01 willGraham01 deleted the wgraham-prhs_cleanup branch December 22, 2022 13:14
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