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...

[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]

Can you explain why? What's the difference between a function defining the fields and a file?

[gadiLhv]

Oh I remember this one, not fondly. I think when a function expression is used, this is set implicitly later, or something.
I remember real trouble wrapping my head around this during the development. Even more so right now. This might be un-necessary, or this might be a life saver. Fact of the matter is, right now I can't even test it...

[gadiLhv]

Let me know your thoughts @thliebig

[thliebig]

I think we need to find out?

[gadiLhv]

If I can remove one redundency, why not. I'd rather deal with this once the CSXCAD issues are resolved, anyway. I'd be happy to test it (crossing fingers that I can). I know for a fact that setting this vector is necessary for everything to work.
What I don't know, is if the if condition is necessary...

[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.