Refactor measurement and printing for future features #302

phdum-a · 2024-11-19T23:48:18Z

Refactoring of measurement and printing system in preparation for multi-excitation support.

Separates outs measurements (to be done in PostOperator only) from printing (to be done by Solvers)
Printing is now change from Postpro functions into small printer classes which also hold a table of results, rather than printing row-by-row. (This is where multi-excitations will add extra columns).
The output path is now handled using std::filesystem more consistently
Measurements in PostOperator are now cached so that printers don't have to worry about that the getter functions might trigger an expensive calculation or one that requires MPI communication.
Add PALACE_MFEM_USE_EXCEPTIONS build flag for unit tests, since catch can't test for aborts.
Other small tweaks and improvements

Note: The output of all solvers after these changes should be identical to the current output*, for easy testing. Changes which alter output will be in subsequent PRs.
[*With one trivial change in the logger for eigenvalues from \omega/2pi -> f]

- export build commands

- Used for testing since Catch does not support testing aborts

- Add tests - Add table wrapper for csv files for open-write-close - Make empty cell NULL

- switch post-processing functions to small classes that store data in tables - replace sequential row printing - separate write during sweep step and final write more clearly

- Requires non-empty output dir - Canonicalize and sync among processors - Introduces palace::fs namespace to stop injecting into std

- Simplify checks since output directory always exists

- Cleaner separation between measurement (PostOperator) and printing (in solvers) - Treat frequency as measurement to be supplied by solver - Add caching system in PostOperator for all measurements, extending port case - Simplify energy measurements using caching system - Store pointer view to lumped/wave port operators in PostOperator where appropriate

- remove redunant default init - init doubles in postop

fedeinthemix · 2024-12-10T10:16:09Z

Hi, since you are refactoring the output format to support multi-excitations, I'd like to ask if would consider adding an option to output S-parameter files in Touchstone format. It's a de-facto standard in circuit simulators and beyond.

phdum-a · 2024-12-10T17:47:32Z

Hi, since you are refactoring the output format to support multi-excitations, I'd like to ask if would consider adding an option to output S-parameter files in Touchstone format. It's a de-facto standard in circuit simulators and beyond.

Dear @fedeinthemix,

Thank you very much for your suggestion. We'll have to think about it more, but right now we don't have plans to print touchstone files directly in Palace.

In cases where people require touchstone files, we have recommended writing a script that reads in the palace csv files from all the separate simulations (e.g. using pandas) and then prints it in the right form. You could also look at scikit-rf that has a touchstone printer and parser.

For Palace, we do want a consistant file formats with other measurements not covered by touchstone, like energy or fields. Also the touchstone spec does not quite cover some cases, like simulating only a subset of columns of the s-matrix.

Please let use know, if there is there anything you think we are missing something in the Palace output. For example, is there something in the touchstone file format / metadata that you can't find easily?

Thanks!

fedeinthemix · 2024-12-11T07:21:22Z

Dear @phdum-a,

Thank you for your answer. I thought that an option to output Touchstone files would be convenient since most RF/mmW passive circuits simulated with palace will probably be used in circuit simulators. But, I understand point, and, as you say, writing a conversion procedure is not a big deal.

Thanks again for your answer.

hughcars

Generally looks pretty good. I have some changes I think need to be made at #325 from hughcars/new_postpro. Additionally given it's a large refactor, mention it in the CHANGELOG.md.

hughcars · 2024-12-05T17:00:39Z

cmake/ExternalMFEM.cmake

@@ -76,6 +76,9 @@ if(CMAKE_BUILD_TYPE MATCHES "Debug|debug|DEBUG")
  endif()
 endif()

+# Replace mfem abort calls with exceptions for testing, default off
+set(PALACE_MFEM_USE_EXCEPTIONS CACHE NO "MFEM throw exceptsions instead of abort calls")


set(PALACE_MFEM_USE_EXCEPTIONS CACHE NO BOOL "MFEM throw exceptsions instead of abort calls")
The BOOL is needed to calm a warning about using the cache. However if you do that, the mfem build then fails due to their parsing of the variable, giving

set(MFEM_DEBUG OFF) set(MFEM_USE_EXCEPTIONS CACHE) set(MFEM_USE_ZLIB YES)

in the mfem cmake cache. I think sticking with
set(PALACE_MFEM_USE_EXCEPTIONS NO) rather than needing to work around mfem's cmake approach.

hughcars · 2024-12-05T17:32:08Z

palace/models/lumpedportoperator.cpp

+    buf.append(buf_a);
+  }
+
+  Mpi::Print("{}", fmt::to_string(buf));


This buffering trick will print everything in one shot rather than scrolling. I don't think its an issue as this goes by so fast anyway as to be indistinguishable from a block print. Only issue would be if anything in here can error and thereby print nothing.

Given there's a few of these buffer printing options, you might consider having an Mpi::Print overload that ingests just a buffer, Mpi::Print(fmt::memory_buffer); that does this under the hood if it's going to be very common. Especially given these buffers are being sent in with no formatting, just directly as strings.

hughcars · 2024-12-05T18:36:21Z

palace/utils/tablecsv.hpp

+  // View to default options in table class, will be set when columns are added to Table
+  ColumnOptions *defaults = nullptr;
+
+  // ----


Unneeded, delete.

hughcars · 2024-12-05T18:36:51Z

palace/utils/tablecsv.hpp

+{
+  friend class Table;
+
+  // View to default options in table class, will be set when columns are added to Table


Nit: Any comment on a new line ends with .,

// New line comment. int v = 0; // Can end without

hughcars · 2024-12-05T19:15:41Z

palace/utils/tablecsv.hpp

+class TableWithCSVFile
+{
+  std::string csv_file_fullpath_ = "";
+  unsigned long file_append_curser = -1;


Comment describing what this variable is used for.

hughcars · 2025-01-07T16:39:10Z

palace/drivers/basesolver.hpp

-  // Common domain postprocessing for all simulation types.
-  void PostprocessDomains(const PostOperator &post_op, const std::string &name, int step,
-                          double time, double E_elec, double E_mag, double E_cap,
-                          double E_ind) const;

  // Common surface postprocessing for all simulation types.


Expand on this documentation, the disctinction between a printer and the performing of the actual measurement is one of the biggest innovations in this PR. A short sentence or two here describing this distinction would be good. Can then reference this in doc string above the printers in the other driver classes.

hughcars · 2025-01-07T16:40:17Z

palace/drivers/transientsolver.cpp

+// -----------------
+// Measurements / Postprocessing


hughcars · 2025-01-07T16:42:41Z

palace/models/postoperator.hpp

+  // Port Contributions: not owned, view onto space_op only, it must not go out of scope
+  LumpedPortOperator *lumped_port_op = nullptr;
+  WavePortOperator *wave_port_op = nullptr;


We do not want these classes referenced within post operator, which shouldn't know about them, other than via a higher level operator (SpaceOperator etc.). I have a refactor that removes the need for this, by passing them in at the relevant call sites, and doing away with the use of optionals/lazy evaluation of the measurement cache. See hughcars/new_postpro

hughcars · 2025-01-07T16:44:24Z

palace/models/postoperator.hpp

+  struct MeasurementCache
+  {
+    std::optional<std::complex<double>> omega = std::nullopt;
+
+    std::optional<double> domain_E_field_energy_all = std::nullopt;
+    std::optional<double> domain_H_field_energy_all = std::nullopt;
+
+    std::optional<std::map<int, double>> domain_E_field_energy_i = std::nullopt;
+    std::optional<std::map<int, double>> domain_H_field_energy_i = std::nullopt;
+
+    std::optional<std::vector<FluxData>> surface_flux_i = std::nullopt;
+    std::optional<std::vector<InterfaceData>> interface_eps_i = std::nullopt;
+
+    std::optional<std::map<int, PortPostData>> lumped_port_vi = std::nullopt;
+    std::optional<std::map<int, PortPostData>> wave_port_vi = std::nullopt;
+
+    std::optional<double> lumped_port_inductor_energy = std::nullopt;
+    std::optional<double> lumped_port_capacitor_energy = std::nullopt;
+
+    std::optional<std::vector<std::complex<double>>> probe_E_field = std::nullopt;
+    std::optional<std::vector<std::complex<double>>> probe_B_field = std::nullopt;
+  };


Do not need optionals on here, many of the measurements are setting the non-evaluable versions to 0.0, and the containers all ready have a notion of size. Removing the optional, and moving all the measurements to occur within the MeasureAll method, can also use concrete values whilst removing the need for mutability aswell. See hughcars/new_postpro

Also, measurement_cache.

hughcars · 2025-01-07T16:46:58Z

palace/models/postoperator.cpp

+void PostOperator::MeasureAll()
 {
-  if (A)
+  ClearAllMeasurementCache();


This can be simplified significantly by using the fact we know when MeasureAll will be called will be the only time to do any measurements. The lazy evaluation isn't necessary. See hughcars/new_postpro

phdum-a requested a review from hughcars November 19, 2024 23:48

phdum-a assigned hughcars Nov 19, 2024

phdum-a force-pushed the phdum/new_postpro branch 4 times, most recently from 2e0b7f0 to c1a8a4b Compare November 22, 2024 23:48

phdum-a added 12 commits December 3, 2024 11:52

Tweak build system

e56664e

- export build commands

Add flag for MFEM to throw not abort

9defb8d

- Used for testing since Catch does not support testing aborts

Simplify printing using more fmt features

03e175c

Add small table class for csv printing

b7e7f69

- Add tests - Add table wrapper for csv files for open-write-close - Make empty cell NULL

Add vdim to interpolation operator

20ed17f

First pass at new postpro printers

26353fb

- switch post-processing functions to small classes that store data in tables - replace sequential row printing - separate write during sweep step and final write more clearly

Clean up while loops

80d2531

Centralizes making output directory

3a779c1

- Requires non-empty output dir - Canonicalize and sync among processors - Introduces palace::fs namespace to stop injecting into std

Change post_dir to fs::path

0f72bd0

- Simplify checks since output directory always exists

Add missing include

8f68b32

Fix table tests with changed default

f58ccbf

phdum-a force-pushed the phdum/new_postpro branch from c1a8a4b to f58ccbf Compare December 3, 2024 19:56

Clang-tidy tweaks

ea3c62d

- remove redunant default init - init doubles in postop

phdum-a mentioned this pull request Dec 4, 2024

Add multi-excitation support to driven solver #309

Draft

4 tasks

phdum-a added 2 commits December 11, 2024 17:28

Bugfix in eigenmode port_Q printer

5a106c8

Fix bug in s-parameters printer

20660d6

hughcars requested changes Jan 7, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor measurement and printing for future features #302

Refactor measurement and printing for future features #302

phdum-a commented Nov 19, 2024

fedeinthemix commented Dec 10, 2024

phdum-a commented Dec 10, 2024

fedeinthemix commented Dec 11, 2024

hughcars left a comment •

edited

Loading

hughcars Dec 5, 2024

hughcars Dec 5, 2024

hughcars Dec 5, 2024

hughcars Dec 5, 2024

hughcars Dec 5, 2024

hughcars Jan 7, 2025

hughcars Jan 7, 2025

hughcars Jan 7, 2025

hughcars Jan 7, 2025

hughcars Jan 7, 2025

Refactor measurement and printing for future features #302

Are you sure you want to change the base?

Refactor measurement and printing for future features #302

Conversation

phdum-a commented Nov 19, 2024

fedeinthemix commented Dec 10, 2024

phdum-a commented Dec 10, 2024

fedeinthemix commented Dec 11, 2024

hughcars left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hughcars left a comment •

edited

Loading