Skip to content

Refactor measurement and printing for future features #302

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

Merged
merged 42 commits into from
Mar 25, 2025
Merged

Conversation

phdum-a
Copy link
Contributor

@phdum-a phdum-a commented Nov 19, 2024

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]

@phdum-a phdum-a requested a review from hughcars November 19, 2024 23:48
@phdum-a phdum-a force-pushed the phdum/new_postpro branch 4 times, most recently from 2e0b7f0 to c1a8a4b Compare November 22, 2024 23:48
@fedeinthemix
Copy link

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
Copy link
Contributor Author

phdum-a commented Dec 10, 2024

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
Copy link

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.

Copy link
Collaborator

@hughcars hughcars left a comment

Choose a reason for hiding this comment

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

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.

@phdum-a
Copy link
Contributor Author

phdum-a commented Jan 22, 2025

Rebased onto main

@phdum-a phdum-a force-pushed the phdum/new_postpro branch 5 times, most recently from 5e4edc6 to f2b6e19 Compare March 18, 2025 01:02
@phdum-a phdum-a requested a review from hughcars March 19, 2025 17:56
phdum-a added 7 commits March 21, 2025 11:02
- 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
phdum-a added 11 commits March 21, 2025 11:02
…ould be complete, does not compile"

This reverts commit 9eb04c3.
- Make unit choice a compile time parameter
- Standardize normalization of inputs
- Template PostOperator over solver type
- Branch on template with constexpr
- Encapsulate printers into helper class PostOperatorCSV
- Tweak table printer interface and force int printing
- Add vector conversion to units
- Return only total domain energy to solver for indicator
@phdum-a phdum-a force-pushed the phdum/new_postpro branch from f2b6e19 to 4e34f6a Compare March 21, 2025 18:17
@phdum-a phdum-a force-pushed the phdum/new_postpro branch from 4e34f6a to 27a33d7 Compare March 21, 2025 18:25
Copy link
Collaborator

@hughcars hughcars left a comment

Choose a reason for hiding this comment

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

Generally looks good to me. Caught some minor bugs, all my suggested changes are on hughcars/new_postpro #356. So feel free to merge that if you agree, then we're good to go.

@hughcars hughcars merged commit c84ad62 into main Mar 25, 2025
20 checks passed
@hughcars hughcars deleted the phdum/new_postpro branch March 25, 2025 17:23
@phdum-a phdum-a restored the phdum/new_postpro branch March 25, 2025 18:13
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.

3 participants