Custom\manual waveguide modes definition for Excitations

[gadiLhv]

As I am getting the hang of the Git protocol, I can start this, seperately. This was (re)based around the original repository master, rather than my one, which has the absorbers. In the future I'll make a habit of opening branches for new features.

I'll go over the remarks from the previous PR

The python interface is wrong? It talks about coors? (coordinates??) and "Weights" but only has the latter?

The line that guided me here was "make as little damage as possible". This was for two main reasons:

1. Avoid code repetitions, where possible.
2. Backwards compatibility with existing code.

Hence, the fields for the mode consist of three columns of coordinates, and three columns of field values: [X,Y,Z,Ex,Ey,Ez]. Replace with H at your own convenience.

The mode should be purely 2D on a plane right? Why does it accept coords in x,y and z and why weights in x/y/z? I would expect only x/y-coordinates (or maybe more general x0 and x1) and a 2D array of weights? That does all make little sense to me?

Three reasons, this time

1. Make as little damage as possible to the original code. This is clearer in openEMS. I know they are seperate, but still. I do a "nearest neighbor" search later, and it makes a lot more sense.
2. The mode functions are also given in all three axes, so it made sense to me.
3. This may sound arbitrary, but it isn't. When I output the geometry data to my mode solver (I am hoping to finalize that some day with the new library I'm using, so I can share it), I am simply snipping it out of the 3D structure. In case the normal is z, it's simple enough. But if the normal is y, I need to rotate the geometry 90 degrees...
   To avoid all this mess, I simply extract all three coordinates. I'll give this some more re-thinking.

I really do not like to give this class possibly large arrays of data like this and none of these values are stored and read into xml? This would be needed if at some point the Matlab/Octave interface want to use this feature too.

I know. I didn't find any built-in XML functions for handling arrays, except for the mesh processing. Care to give me a hint?

I really do not like that this is implemented twice in the excitation and the probe! (Large) redundant code is a no-no!!

I'll think how to implement these functions just one time. Missed this remark while re-basing the code.

Create a new file format (maybe plain csv or json or hdf5) that holds all required info about the modes, e.g. x and y grid coordinates and the mode on a 2D plane
The excitation and probes use a function SetWeightFromFile with some additional info's (see next point)
Supply an additional translation (and maybe rotation) information to allow to reuse the mode file for multiple ports at multiple locations and rotations

This will take some figuring out. CSV is my weapon of choice, I guess. Easy to manipulate both in M-script and python. If I make this a 2D file, how do I handle the coordinate shift? Rotation is the easy bit, but where is (0,0)?

This approach would allow openEMS to have one new (utility) method to read and interpolate this custom mode for both the excitation and the probe and thus avoid redundant code and memory usage

[gadiLhv]

Hey @thliebig

Took me long enough, but I have a parser for CSV files with E and H data.
Is it best to put it in CSXCAD or OpenEMS? I'm leaning towards CSXCAD because the method

double CSPropExcitation::GetWeightedExcitation(int ny, const double* coords)

is used by openEMS.

[gadiLhv]

Hey @thliebig
I also found another issue.
There is no proper storage container for the propagation direction in CSPropexcitation.
The way it worked so far, was that only two strings for mode functions were input, so the software didn't enforce anything.

In order to overcome that, I have two possible courses of action:

1. Start using the SetActiveDir methods in the CSPropExcitation class. I checked, they are used only in TFSF excitation.
2. Add a new container.

I'll go with #1 right now, but if you have a different idea I'd love to hear.

Cheers

[gadiLhv]

Hey @thliebig

Finally got a hold of this, or at least I think. Rebased, tested, the works.
I'm guessing you will have some corrections and questions.
Also, a similar post will be in the openEMS counterpart of this.

Cheers

[gadiLhv]

Hey @thliebig. This was done in parallel to the openEMS PR, as usual. Let me know if this is more in the direction you wanted

[tmpusr123]

@thliebig found an opportunity to take a look by any chance?

[thliebig]

Yes I had a first look. I have not yet tried to build and test this. Looks good so far I think. Next I will have a closer look at the openEMS counterpart first.

[gadiLhv]

During adding another example, I found a bug in the transpose of the file parser. Fixed in the last push. Forced push for rebase purposes, as usual...

[thliebig]

Additionally there are many random blank lines in odd places?

[gadiLhv]

I pushed the changes @thliebig. I'm assuming it's just an initial review...

[thliebig]

Yes, but I think it is looking okay. I need some closer look at the ModeFileParser class.

You may add some unit tests in the CSXCAD/python/tests/test_CSProperties.py ? Maybe even supply very simple *.csv and test the interpolation? That way we can make sure it works as expected and make sure it stays that way?
You can run them with:
python -m unittest test_CSProperties.py

[gadiLhv]

I'm having serious trouble with the python bindings. There was a change to update_openEMS.sh, and now the python binding are compiled using /scripts/build_python.sh.

I can't find the logs anywhere to find the errors.

1. It took a minute to find out how to bypass the venv.
2. The venv doesn't install it's own packages.
3. I can't find the error log for the python build now.

[thliebig]

Very testing and development you should really not use the update_openEMS.sh. That is meant to build and install everything. What you want is only build what you are actively working on!

If it is only the python bindings, cd into the the specific python path, here CSXCAD/python and build the python extension from here.
I am still using my old ways that I'm sure are not the most "elegent" any more:

export CSXCAD_INSTALL_PATH=/home/thorsten/opt
python setup.py build_ext -I /home/thorsten/opt/include -L /home/thorsten/opt/lib -R /home/thorsten/opt/lib
python setup.py install --user

You have to adept the path obviously and @biergaizi is most likely yelling at me for not using a venv and pip 😏

[gadiLhv]

I did that. It compiled and everything, but when I run the unit test it doesn't see my additions.
Also, there is a segmentation fault on test_dump.
Really don't know how to proceed.
When I try to use plain update_openEMS.sh, I'm getting some error about Cythom module missing. It's not.

Let me know what to do. I'm really on the verge of ditching this.

[thliebig]

but when I run the unit test it doesn't see my additions.

What do you mean? You are in the "tests" folder when you run python -m unittest

[gadiLhv]

I wrote everything and tested as best as I could. I can't install with the new workflow, and I'm not going to try anymore. The new installation script after @biergaizi 's latest "re-write" doesn't work with my additions. Tried rebasing and everything. Got no idea why. The C++ part works. The python, doesn't. There is no feedback, logs or anything that I can find.

When I tried this:

export CSXCAD_INSTALL_PATH=/home/gadi/opt/openEMS
python setup.py build_ext -I /home/gadi/opt/openEMS/include -L /home/gadi/opt/openEMS/lib -R /home/gadi/opt/openEMS/lib
python setup.py install --user

The python bindings compiled fine. They were installed somewhere. When I tried to run the unit tests, it referred to the previous version. Why? I don't know.

~ Throwing the white flag ~

My collegue said he wanted to try from his side. @tmpusr123, it's up to you now.

[thliebig]

Well I was able to checkout, rebase and build it just fine. Not sure whats up with your setup (or mine). The tests obviously can not work like this yet and they are already a bit deeper than I intended.
Maybe you just remove the last commit, I will do my final review and than we call it good and I will maybe have a look at this unittest later. My intention was more to test the GetWeightedExcitation C++-function, but I realize that it does not have a python interface to begin with...

[gadiLhv]

I can port GetWeightedExcitation to python, but I can't test. It's pretty much the same though...

This is the line it uses:

m_ModeFile.linInterp2(loc_coords[nPyp],loc_coords[nPypp],Wny)

With this test, you can use both interpolants.

I'm guessing this entire effort was to better use VScode. I use Eclipse for all of my work. As far as the rest of the setup, it's the same Linux VM I've been working with since the first lumped element PR.

[thliebig]

I can port GetWeightedExcitation to python, but I can't test. It's pretty much the same though...

That does make little sense, just "unpush"? The last commit and we go from there...

[gadiLhv]

I can port GetWeightedExcitation to python, but I can't test. It's pretty much the same though...

That does make little sense, just "unpush"? The last commit and we go from there...

I edited a second later.

This is the line it uses:

m_ModeFile.linInterp2(loc_coords[nPyp],loc_coords[nPypp],Wny)

With this test, you can use both interpolants.

[gadiLhv]

I'll refine.

In the GetWeightedExcitation method, the call to the interpolant is here:

https://github.com/gadiLhv/CSXCAD/blob/ffe7a18c5a59d198f74453841d66eb779f924a1c/src/CSPropExcitation.cpp#L217C1-L219C66

// Get weights in both directions
double Wny[2] = {0.0,0.0};
m_ModeFile.linInterp2(loc_coords[nPyp],loc_coords[nPypp],Wny);

While the "shift" to the local coordinate system is done in openEMS, as it relates to the primitive that defines the excitation.

https://github.com/gadiLhv/openEMS/blob/7d5823a266c027423f121ef95c5bd022db723c0b/FDTD/extensions/operator_ext_excitation.cpp#L196

if (!shiftCoordsForModeFile(volt_coord, highestPriorityPrim))

This may not be the optimal solution. However, considering that in the rectangular waveguide port, this is done in the openEMS python interface, I figured that doing it in the C++ code would even be a bit friendlier. This is how it's done in the original master branch, BTW::

name_P = '({}-{})'.format(xyz[self.ny_P], self.start[self.ny_P])
name_PP = '({}-{})'.format(xyz[self.ny_PP], self.start[self.ny_PP])

So testing GetWeightedExcitation here is meaningless, pretty much. Truth be told, most of the work is actually done pre-process, in the mode solver...

[tmpusr123]

Hi @thliebig, happy new year — did you have a chance to build/test this locally, or are there any more blockers to merging?

[thliebig]

Sorry but the test are still failing and I tried to fix some obvious issues, but the tests still fail...
Please get this fixed and get the install workflow figures out?
Ideally also rebase on the current master and force push an update too

[gadiLhv]

Oh well
I'm hoping them LLMs will help me wrestle with this. I'll get to this probably next week.
Appreciate the effort, @thliebig. Will finish this soon enough.

[gadiLhv]

It wasn't a simple rebase, but with the help of friendly robots, I managed.

The results for this
python -m unittest test_CSProperties.py
is this

.............
----------------------------------------------------------------------
Ran 13 tests in 0.042s

I verified that it works with openEMS, as well, although that's irrelevant here.
Let me know if I missed something, @thliebig.

Cheers

[gadiLhv]

I noticed that the linux tests fail because of lack of scipy. What should I do?

[thliebig]

We should avoid the scipy dependency (only for this). You can just check against fixed numbers instead of using the interpolator? Usually putting fixed numbers in code is a no-no. But in this tests it is okay as we expect the result to always be the exact same number no matter what.
Maybe comment out the generating lines thus in the future new numbers can be created and their "correctness" checked.

[gadiLhv]

I checked. Numpy has linear interp built in. I'll write NN manually.

Will finish this by tomorrow.

[gadiLhv]

Green lights all around, @thliebig

[gadiLhv]

To try and not mix further the already mixed salad here.

@biergaizi, @thliebig, if the only issue to make this work with openEMS better is to rename the mode parser as CSModeFileParser and add the CSXCAD_EXPORT predef, say the word.
I'll expose as public as little as possible, per your suggestion.
I saw there Re additional exclusions for this logic, e.g. ParameterCoord.

Let me know

[gadiLhv]

@biergaizi, I reviewed the code. Everything that is exposed is necessary.

@thliebig, let me know what is the best course of action here. Simply add CSXCAD_EXPORT or rename the class and files first.

[thliebig]

Why would you need to rename something? It just sounds like you need to add some "CSXCAD_EXPORT" at some places?
But I need to have a closer look at the Windows error message I guess

[thliebig]

As far as I see it the class "ModeFileParser" is not exposed. Just add a "CSXCAD_EXPORT" in front of it and all should be good.... maybe 😏

[gadiLhv]

Can do!
If you can, please trigger a check in openEMS as well, so we know if this was enough.

[thliebig]

Seems good now

[biergaizi]

@thliebig: As far as I see it the class "ModeFileParser" is not exposed. Just add a "CSXCAD_EXPORT" in front of it and all should be good.... maybe

Please reconsider, and address my comment in thliebig/openEMS#175 (comment). 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. But this class does not follow CSXCAD's naming conventions, and it will be too late to change it if it's accepted as a stable API.

[thliebig]

@biergaizi, sorry I somehow missed that comment completely. I agree with your summary there but it exposes more like a different issue. We need the "CSXCAD_EXPORT" to make (the current implementation) equal to whats available for Linux and fix the current issue. So it needs to be there. But we should address the points you made too.
I think we should go with points 1+2 for now by making it a better API. There is not really a way to truly make an API "internal" in C++ right?

But another point I would like to look at is, why does openEMS need to know all this at all? Ideally openEMS just wants the mode profile and does not want to know how and where that came from (function vs file)? Not sure if that is feasible or not. But if we could make that unnecessary, we could make the ModeFileParser truly "internal" maybe....

But it does not hurt anything if @gadiLhv could look at and fix this to the CSXCAD naming convention (fully internal or not), thanks.

[gadiLhv]

I'll address what's relevant to me, if you may.

But another point I would like to look at is, why does openEMS need to know all this at all?

I'll start with the complicated part. The mode port factually has 2 parts.

1. Excitation - This part is 100% internal in CSXCAD. High fives all around. The exception is the two openings I left in CSPropExcitation, forgot the unit tests.
2. Field probe + mode match integral. This is where it gets messy. There is no internal functionality available here. If you look in openEMS (@theliebig master branch), there is direct access to CSFunctionParser here. So I followed suit. The other option requires more significant changes than the intentions of this PR.

But it does not hurt anything if @gadiLhv could look at and fix this to the CSXCAD naming convention (fully internal or not), thanks.

Except for renaming it to something like CSModeParser, anything else?

[thliebig]

Well name of the files, name of the class and name of the methods. As well as maybe checking which methods need to be public and which one could be made private.

[gadiLhv]

Well name of the files, name of the class and name of the methods. As well as maybe checking which methods need to be public and which one could be made private.

Applied this, to the best of my understanding.