Custom\Manual waveguide mode addition, for arbitrary waveguide structures

[gadiLhv]

This one was rather tricky. Took me a good long while to figure out what functionality I should access in openEMS.

---

I'll start from the tricky part, in ports.py. This is where openEMS and CSXCAD are intertwined. I needed some sort of validation of the inputs, to add this functionality to WaveguidePort. Until the CSXCAD PR isn't finalized, this can't be, either. Unless you want to neglect the python functionality for now.
https://github.com/thliebig/openEMS/compare/master...gadiLhv:openEMS:WaveguideModes?plain=1#diff-fe6a56ae621157b6691266fcc774ca626b01b38cb611a5b21e623b9a9d3493c7R382-R391

---

The only bit of redundancy may be this:
master...gadiLhv:openEMS:WaveguideModes#diff-53e7f365700486b254789d1b8ff10d0073b4bfd6b44e4b5823f72f2192d1b96fR282-R305

where this doesn't exist anywhere else in openEMS, rather in CSXCAD. If I understand correctly, you would rather some sort of field file handling class for this.

---

Here is the clever bit, though. I didn't write anything to the engine extension. It's actually done in the GetWeightedExcitation function, here:
https://github.com/gadiLhv/openEMS/blob/4195be2c83c118ea9f26ddf8eff90715744d7a68/FDTD/extensions/operator_ext_excitation.cpp#L205

and here:
https://github.com/gadiLhv/openEMS/blob/4195be2c83c118ea9f26ddf8eff90715744d7a68/FDTD/extensions/operator_ext_excitation.cpp#L260C9-L260C84

[gadiLhv]

Hey @thliebig

What I couldn't (easily) overcome here was in the excitation extension, I couldn't pass just the file name.
This is due to the fact that the excitation initialization is done throughout the grid, rather than primitive-by-primitive.
This forced the file parsing to be done within the CSPropExcitation itself, but it's done with the file parse, while the only input given is the file name.

Other than that, pretty straight forward. Hope I didn't mess anything else up, too bad.

Cheers

[gadiLhv]

Hey @thliebig

First of all, thank you for merging the local obsorber PR. I hope it will be useful for other things, as the SIBC/MUR-SA is a pretty cool addition.

This PR went through rather extensive changes (along with it's CSXCAD counterpart) so let me know how do you find it.

Cheers

[thliebig]

In general it would also be nice to eventually add an Matlab/Octave interface and example too? But maybe once this is done and merged...

Does this work for cylindrical coordinates? But as not many (if any) use this is CS, it is not a deal-breaker...

[thliebig]

Please avoid trailing whites-spaces... check if you can configure your editor?

[gadiLhv]

I really don't know what you meant here. There was no trailing white spaces option enabled (I used Eclipse). I also didn't see anything unclear. The line space is for readability, and the tab is because of the next line.

I also checked to see if there are any un-necessary spaces there, as well. Didn't find anything.

[gammaxy1]

Lines 358 and 437 were both empty before and now have 8 spaces.

[thliebig]

Okay maybe trailing was a bit misleading, empty lines should not have white-spaces too, just empty please.

[gadiLhv]

Hey @thliebig

I will review them this week. Seems straightforward enough.

Regarding the Octave/Matlab interface, I will put that on the task list. If you insist, I'll do it now, but I really want to organize my Repo a bit. This on-job git training was very deductive, but I really want to keep it cleaner from here on forward. I'm more fluent in m-scripting than I am in Matlab, so should be simpler.

I need to think about how to do it in cylindrical CS, as I never really used it. I need to figure out the deal with the mesh there. Also, the interpolation schemes I wrote for the mode parser hardly fit that. Add it to the pile, I guess 😆.
I Will be happy to sharpen my wits on that. Once the infrastructure is laid down, should be easier.

Hardly a finalized version, but some of my initial mode solver codes are here:
https://github.com/gadiLhv/FEMopenSourceAdventures
The end code will either be in DolfinX or just use a new library called Emerge.

[thliebig]

As I said, keep both points (Octave interface and Cylindrical CS) for later. I would almost think that Cylindrical Coordinates might just work as is (of course with matching csv data)... maybe I will look into it... later...

[gadiLhv]

I've finished the changes requested. Since the local absorbers were already merged, I'd like to add another example with a different T-Line. Maybe remake the MSL case, or add a CPW one. I'll push when done.

[gadiLhv]

Added another example, for CPW. I couldn't verify it against 3rd party software, regretfully, yet.
It's weird that adding PEC blocks makes the convergence worse. I wonder how all the other software do it.

[gadiLhv]

Here, a nice video

test_CPW.mp4

[biergaizi]

Great video. What settings did you use to render the 3D volume correctly? This is worth documenting, the best thing I managed to do in ParaView is to render a slice from a volume rather than a full volume.

[gadiLhv]

Hey @biergaizi
This is paraview. Simply sliced it down the middle, and added an additional slice at y=0.001

[gadiLhv]

I changed the new test script a bit + made more appropriate mode example files.
Although there is almost no resonance here, I still see very high reflection in the exciting port. I verified that it isn't a bug in how the mode are accepted. I think it's an issue of fringing fields from the waveguide edges, but I have no way to prove this.

[gadiLhv]

Hey @thliebig

Any other changes necessary for thie PR?

[tmpusr123]

@thliebig can this be merged yet?

[thliebig]

Give me some more time to start another review soon hopefully...

[tmpusr123]

Hi @thliebig -- any chance you might have an opportunity to merge this soon, to expand support for waveguides?

[biergaizi]

While I'm not the project owner and can't do any code review on the algorithm, but as the build system and CI/CD maintainer I must point out that this Pull Request should be rebased against the latest master branch to allow Continuous Integration to work correctly, because major changes in the build system has occurred in the meantime.

Please run:

# add an alternative origin named upstream
git remote add upstream https://github.com/thliebig/openEMS
git fetch upstream master

# go to your local feature branch
git checkout WaveguideModes

# save a duplicate backup branch it in case the rebase went wrong
git branch WaveguideModesBackup

# rebase the local feature branch against the upstream git branch
# to reset the time of divergence to the last commit, to avoid a complicated
# fork-in-the-middle merge
git rebase upstream/master

# solve any merge conflicts here, should be none
# use git add and git rebase --continue to mark resolution if any conflict does raise

# overwrite the current Pull Request
git push --force

If the rebase succeeds and the CI pipeline passes, your next problem is to fixup/squash/reword all isolated fixes together, because no future developer is interested in reviewing facts such as how the folder example has been changed in the draft. All commits ideally should compile because broken commits prevents developers from testing individual commits in git bisect.

# make sure you still have the WaveguideModesBackup branch
# because it's very easy to accidentally destroy your code during rebase
# perhaps backup it again
git branch WaveguideModesBackupAfterFirstRebase

# now let's start an interactive rebase against the upstream
git rebase -i upstream/master

Now git should start a text editor. Because a lot of editing is involved, make sure the default editor is easy to use. If not, you can change it using the shell variable such as export EDITOR=vim, export EDITOR=emacs, or whatever. The editor should display the following content:

pick 9b855ad Compiles, Runs and ready for PR Added test scripts
pick 5cfae9f Started integration of mode parsers
pick 8ed5b0c Forgot to approve processmodematch.h
pick d4a4bf5 Fixed coordinate shifting in processmodematch
pick 318f9e5 Compiles, untested!
pick 827cc90 Fixed ports.py
pick 7250eda Debugging operator_ext_exciation
pick c282ca4 Debugged excitation modes
pick 4f7a0bb Fixed final bugs in ports.py
pick 8c254e6 Changed example file folder location
pick ff9daed Changed Example file folder location
pick 35bb679 Fixes per @thliebig requests Added another example for CPW, propagating in a different direction

If you want to merge a commit to its previous commit (common when the next commit fixes a mistake in the previous one), change the keyword pick to fixup. If you want to rewrite the history message, change pick to reword, etc. If you want to delete a commit, delete the line (this is when accidents often happen). If you believe the rebase commands are seriously incorrect, delete everything in the file and save an empty as a hard abort (missing lines = dangerous, all-empty file = safe).

Any new feature development should be carefully reintegrated into the main branch. I recommend rebase incrementally, starting from the easiest tasks.

This is what I would do - keep a few milestones, and merge unnecessary fixes upwards.

pick 9b855ad Compiles, Runs and ready for PR Added test scripts
pick 5cfae9f Started integration of mode parsers
fixup 8ed5b0c Forgot to approve processmodematch.h
fixup d4a4bf5 Fixed coordinate shifting in processmodematch
pick 318f9e5 Compiles, untested!
fixup 827cc90 Fixed ports.py
fixup 7250eda Debugging operator_ext_exciation
fixup c282ca4 Debugged excitation modes
fixup 4f7a0bb Fixed final bugs in ports.py
fixup 8c254e6 Changed example file folder location
fixup ff9daed Changed Example file folder location
fixup 35bb679 Fixes per @thliebig requests Added another example for CPW, propagating in a different direction

When you're done, save the file. After saving, you have three clean commits.

9b855ad Compiles, Runs and ready for PR Added test scripts
5cfae9f Started integration of mode parsers
318f9e5 Compiles, untested!

Because the last commit message "Compiles, untested!" is now outdated and would mislead futere developers, you should rewrite the commit log of the last commit to reflect the actual changes. What I'd do is to write a full introduction of the feature in the last commit.

You can start a rebase again git rebase -i upstream/master and use the keyword reword to do so. As a shorthand, you can rewrite the message of the last commit using:

git commit --amend

Once you're done, do a force-commit to overwrite the PR:

git push --force

If you use consistently this technique during actual development, you can ensure every single commit is working and also create the impression that there's never any build failures in the project like the Ministry of Truth (but don't abuse it, rewriting history in main/master is disruptive, you should only rewrite your private feature branches).

[thliebig]

Well the alternative and what I would have most likely done is to just squash merge this after review. Said said, if you do not want to rewrite the history and are fine with a squash merge, you can at least rebase to the latest master to see if the CI system finds any build issues...

[gadiLhv]

You want me to rebase, sure, no worries.
I'll dump all the unnecessary stuff in the force push.
I'll do it tonight

It won't ever build until the CSXCAD PR is merged. It's based on stuff programmed there...

[thliebig]

Whats missing here? After that if/else use_function_expr can never be None? I think the else should have been a check for the file mode?

[gadiLhv]

True that. Fix will come in the nearby push in this form:

        use_function_expr = None
        if (self.E_func is not None) and (self.H_func is not None):
        	use_function_expr = True
        elif (self.E_WG_file is not None) and (self.H_WG_file is not None):
        	use_function_expr = False

[thliebig]

Does not seem fixed in my latest pull?

[thliebig]

I am not sure that I am a fan of checking the file ending? Also a *.dat file could be a valid mode file? Or even a file without any extension at all?

[gadiLhv]

The extension is a design choice.
The amount of times I tried to use a general csv as a far field source, a CST graph export as s-parameters, etc. I feel like enforcing it to be a CSV will cause the user to actually verify before.

Should you demand, I'll remove this input check, no problems.

[gadiLhv]

Removed, in the next push.

[thliebig]

also not changed?

[thliebig]

It won't ever build until the CSXCAD PR is merged. It's based on stuff programmed there...

You are right, no need to therefore at this point. I will squash merge and build test locally and let CI check afterwards and fix anything if it is found.

Please just look at my initial review comments ..

[biergaizi]

It won't ever build until the CSXCAD PR is merged. It's based on stuff programmed there...

I see.

As a tip, the openEMS CI/CD pipeline is now enabled globally for all branches, after rebasing your openEMS feature branch, CI/CD will now try using the master branch of fparser and CSXCAD under your personal account to build it, and the result will be shown in your personal repo. So you can first fork fparser, then fork CSXCAD and rename the true master branch to another name, copy your feature branch to the master branch and push it. Then make a dummy commit using git commit --amend to trigger an openEMS rebuild, you can now run the CI/CD process using your personal copy of fparser, CSXCAD and openEMS.

I need to document that in my website PR...

Perhaps it can be improved further in the future, such as falling back to the upstream if the repos don't exist, or use the branch of the same names in two different repos...

[biergaizi]

I just opened a TODO proposal named Enhanced Dependency-Aware Multi-Repo CI/CD Pipeline Logic to make things easier for both @gadiLhv and reviewers.

[gadiLhv]

Please just look at my initial review comments ..

Will do, @thliebig. It requires actual thinking, so I'll get to it tomorrow.

I just opened a TODO proposal named thliebig/openEMS-Project#430 to make things easier for both @gadiLhv and reviewers.

I'm actually more confused by this, somehow. But I'm not a prime example of a developer. Just happen to like computational electromagnetics.

[biergaizi]

I'm actually more confused by this, somehow.

What I basically said is that, if the idea is implemented in the future, two simultaneous changes targeting CSXCAD and openEMS will be tested together at the same time, if both Pull Requests come from the same developer, and use the same local branch names. This will eliminate the problem of not being able to run meaningful CI in a stalemate like this PR (I've also encountered the problem many times).

[biergaizi]

My "Pull Request Ganging" proposal has just been implemented. Please try a dummy useless force push to force the CI/CD pipeline to rerun:

git commit --amend  # without changing anything, just save
git push --force

After a force push, your openEMS Pull Request will be tested together with your latest CSXCAD Pull Request commits (because both have the same WaveguideModes branch name).

[thliebig]

I can request a re-run manually

[gadiLhv]

I couldn't figure out what went wrong with the MSVC test. Any hints, @biergaizi & @thliebig?

Nice to see some green lights on this, for a change

[biergaizi]

I couldn't figure out what went wrong with the MSVC test.

Undefined reference due to unexported library functions. On Windows, all symbols are private by default. To make a symbol available to other applications in a DLL, you must mark the class or function as CSXCAD_EXPORT (in openEMS, it's OPENEMS_EXPORT), which is an alias of __declspec(dllexport).

For example, the class CSPrimBox has the signature:

class CSXCAD_EXPORT CSPrimBox : public CSPrimitives
{
// ...
}

But your class ModeFileParser does not export this symbol, so it's unavailable as a public function for openEMS (public in the operating system's .so/.dll sense, not the C++ sense). This is why you when you try to use ModeFileParser from openEMS you get:

lld-link: error: undefined symbol: public: void __cdecl ModeFileParser::clearData(void)
>>> referenced by CMakeFiles\openEMS.dir\Common\processmodematch.cpp.obj:(public: virtual void __cdecl ProcessModeMatch::InitProcess(void))

lld-link: error: undefined symbol: public: bool __cdecl ModeFileParser::parseFile(void)
>>> referenced by CMakeFiles\openEMS.dir\Common\processmodematch.cpp.obj:(public: virtual void __cdecl ProcessModeMatch::InitProcess(void))

lld-link: error: undefined symbol: public: void __cdecl ModeFileParser::linInterp2(double, double, double &, double &)
>>> referenced by CMakeFiles\openEMS.dir\Common\processmodematch.cpp.obj:(public: virtual void __cdecl ProcessModeMatch::InitProcess(void))

---

But we should be careful here. Everything in ModeFileParser will permanently become CSXCAD's public API, and in theory anyone who uses CSXCAD will be able to link against it and use it (even on Unix, because they're public by default). Are you sure that you want everything in ModeFileParser to permanently become the public API of CSXCAD?

The class name is not following CSXCAD naming convention of CSClassName, and the member function coding style is in Class.camelCase, while all CSXCAD's public API uses Class.CapsForEveryWord. If this becomes the public API, it will be confusing to downstream users, so I suspect ModeFileParser is meant to be an internal function?

I think we have several choices:

1. Full public API integration: Redesign ModeFileParser to be a true CSXCAD API, so it's consistent with all other CSXCAD functions.
2. Only expose bare minimum as the public API: Wrap the public part of ModeFileParser as CSModeFileParser, with just enough functions for the public (including openEMS) to use the mode-matching feature, hide all implementation details.
3. Classify it as an internal API, without future public integration plan, by design: Export ModeFileParser, go with the code as-is, but mark ModeFileParser as "private to openEMS, do not use" in the documentation to make sure no actual users will call it, perhaps by creating a special namespace, with the future plan to make it remain private in the foreseeable future.
4. Classify it as an internal API, but open to future public API integration plan: Export ModeFileParser, go with the code as-is, but mark ModeFileParser as "experimental, do not use" in the documentation to make sure no actual users will call it, with the future plan to make it a stable part of the public API.

[gadiLhv]

#1 and #2 are a fundamental change. Both for this and the equivalent PR.
Let me know what to do, @thliebig.

[gadiLhv]

The fixes here were very minor. Applied, as well.

[gadiLhv]

Hello again, @thliebig

A Matlab/Octave script was added to matlab/examples/transmission_lines

The m-script functionality is a bit more limited, as it originally supported only TE modes. I matched the same capabilities.

I translated only one script of the three, but it should be the same for the rest.

[tmpusr123]

@thliebig anything more needed here?

[thliebig]

@gadiLhv I hope not, but I want to do a (hopefully) final code review and testing. But I need to find the time and motivation to do it.

[thliebig]

Post python-Test script do not work anymore due to multiple issues... Please fix them. Thanks

[gadiLhv]

Post python-Test script do not work anymore due to multiple issues... Please fix them. Thanks

Can you tell exactly what didn't work? I ran the CSXCAD unit tests, and for me, the update_openEMS.sh script worked.

gadi@gadi-virtualbox:~/Repositories/openEMS-Project$ ./update_openEMS.sh ~/opt/openEMS --python --python-venv-mode=disable
enabling Python Extension build
setting install path to: /home/gadi/opt/openEMS
logging build output to: /home/gadi/Repositories/openEMS-Project/build_20260221_223019.log
configuring /home/gadi/Repositories/openEMS-Project via CMake ... please wait
compiling /home/gadi/Repositories/openEMS-Project ... please wait
Building python modules ... please wait
build successful, cleaning up tmp dir ...
 -------- 
openEMS and all modules have been updated successfully...

% add the required paths to Octave/Matlab:
addpath('/home/gadi/opt/openEMS/share/openEMS/matlab')
addpath('/home/gadi/opt/openEMS/share/CSXCAD/matlab')

Have fun using openEMS

[thliebig]

I did not mean the unit tests, I meant the full python demo's. I think they also would have better been added to an examples folder. "python/Tests/Coax_W_WG_Ports.py" and CWP_* is what I meant.

[gadiLhv]

I did not mean the unit tests, I meant the full python demo's.

What should I run to recreate the issues?

I think they also would have better been added to an examples folder. "python/Tests/Coax_W_WG_Ports.py" and CWP_* is what I meant.

This I understood even less. It's in this folder right now. What would you rather?

[thliebig]

What should I run to recreate the issues?

Just run and fix them both? I think I only tested one and there were undefined variables and not up to date function calls.

[gadiLhv]

Sure, will take care of it tomorrow.

[gadiLhv]

Solved the issue in the Coax example and improved the performance of the CPW. Also added a disclaimer with things to fiddle with.