-
Notifications
You must be signed in to change notification settings - Fork 68
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
Conversation
2e0b7f0
to
c1a8a4b
Compare
c1a8a4b
to
f58ccbf
Compare
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! |
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. |
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.
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
.
d3e0ce8
to
4e91bcf
Compare
Rebased onto main |
4e91bcf
to
bf5de66
Compare
5e4edc6
to
f2b6e19
Compare
- 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
…ould be complete, does not compile" This reverts commit 9eb04c3.
- Make unit choice a compile time parameter - Standardize normalization of inputs
- run format
- 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
f2b6e19
to
4e34f6a
Compare
4e34f6a
to
27a33d7
Compare
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.
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.
Refactoring of measurement and printing system in preparation for multi-excitation support.
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]