thliebig
Adding localized Mur\SA-Mur absorbers
There are two additions in this pull requests:
- The ability to add Mur B.C. locally (not necessary on the edge) as a primitive. Some use cases:
- Horn antennas, such that the input port will not be on the boundaries on the simulation. This should enable better radiation pattern calculation
- Coaxial to PCB transitions.
This allows also to place PEC blocks near the absorber, shielding from leakage, like in this coaxial example
- Load externally calculated modes, that cannot (or are very hard) be obtained analytically, or very hard to excite with discrete ports. For example:
- Corrugated waveguides
- Dielectrically loaded waveguides
- CPW
I think I should also add an example script for this.
All original functionality is maintained. All functions can be used as before, as well. I didn't know how to load matlab arrays, so I only added python functionality.
First some general comments:
- CSXCAD is not an extension of openEMS. CSXCAD is meant to be a base for any kind of numerical (EM) simulation. For example it was originally created for a static field solver and only later was adopted by openEMS. Thus it is my goal to keep it as independent as possible from any specific simulation tool or even method.
- I would really prefer this PR to be split into the two (one for each feature). I can see that there is still some work to be done before I can merge this and thus I would ask that you please create two new PR as described below.
1) About the new absorbing property:
- In summary: this should not be too difficult to get this into an acceptable state but please make it it's own PR!
- Why is this called a BC (boundary condition)? I think this is just an absorbing material? Just skip/remove the "BC"?
- Maybe this should be derived from CSPropMaterial instead of CSProperties? This way the Absorber could also access a "base material"? This might be useful if this absorber (material) maybe someday should also work when put at a material (non-air) interface? Not sure about that but I think we have to somehow make clear that this is a material?
- SetBoundaryType should be renamed to SetAbsorberType and maybe accept a general string instead of an enumeration/int. Because "MUR_1ST_1PV" and "MUR_1ST_1PV_SA" is very implementation specific and CSXCAD should be more method generic (see above statement)? openEMS (or any other solver) then could parse this string and apply the appropriate implementation.
- SetPhaseVelocity, Do we really need this? The material should now its wave number (eps_r and mue_r) and e.g. openEMS know its target/center frequency? That said it would be better if the EM-tool would just calculate this? But it would also be fine to keep it.
2) About the new custom mode profile:
- In summary: This needs much more work and maybe discussion. Maybe at first here and then in a separate PR.
- Pyhton (and Matlab) are only interfaces/wrapper around CSXCAD which is a C++ library. Thus all important functionality needs to be implemented in C++ and not in python!
- The python interface is wrong? It talks about coors? (coordinates??) and "Weights" but only has the latter?
- 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?
- 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 really do not like that this is implemented twice in the excitation and the probe! (Large) redundant code is a no-no!!
Here is my proposal to improve on this:
- 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 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
Summary
I really like both features and I think they would be nice to include into CSXCAD and openEMS. If we work on the issues and improvements I have outlined we can make this happen!
Thanks for your contribution and I really hope we get this sorted!
That was fast! First of all, thank you for taking the time to write this.
Which of the two options do you seem fit:
- Split into two PRs, and then have a separate discussion?
- Discuss the necessary changes, and then split the PRs
Well ideally the new PR's are already in good shape? Thus if more discussion is required we can still do it now and here? And maybe we start with 1) first and wait until we have that done? Maybe in openEMS too?
Ok, I'll start by splitting this into two PRs. Both of them will affect openEMS and CSXCAD. After that I'll start addressing all the points...
Hopefully, soon, but who am I kidding...
As the localized absorbers are a pre-requisit for everything, let's start there.
I'll comment on the relevant portions, for now. When we get past this PR, I'll move to the custom modes and the relevant comments. Please don't delete them or edit them out, I'll use them later. Promise!
CSXCAD is not an extension of openEMS. CSXCAD is meant to be a base for any kind of numerical (EM) simulation. For example it was originally created for a static field solver and only later was adopted by openEMS. Thus it is my goal to keep it as independent as possible from any specific simulation tool or even method.
Acknowledged, appologies.
In summary: this should not be too difficult to get this into an acceptable state but please make it it's own PR!
Agreed, and here we are :)
Why is this called a BC (boundary condition)? I think this is just an absorbing material? Just skip/remove the "BC"?
Because functionally that's what it is. It's working on a surface. Maybe that is a hint as to how to implement this later?
Maybe this should be derived from CSPropMaterial instead of CSProperties? This way the Absorber could also access a "base material"? This might be useful if this absorber (material) maybe someday should also work when put at a material (non-air) interface? Not sure about that but I think we have to somehow make clear that this is a material?
Maybe this is more suitable for resistive sheet, then? Even if I define a volume, only one of it's faces will be set with the proper engine extension. What bothers me most about using CSPropMaterial, is that I need to figure how to not harm existing functionality. I am always scared to deal with CSPropMaterial, as it is the most complex class there.
SetBoundaryType should be renamed to SetAbsorberType and maybe accept a general string instead of an enumeration/int. Because "MUR_1ST_1PV" and "MUR_1ST_1PV_SA" is very implementation specific and CSXCAD should be more method generic (see above statement)? openEMS (or any other solver) then could parse this string and apply the appropriate implementation.
I'll need to figure how to send strings, but Ok. I just really like enumerators, I find them more tidy. In the future I really want to add surface impedance BC (SIBCs). I don't know how efficient that is. I also want to implement something called Modal absorption, but I failed even in the basic 2D simulations with this, so I'll stick to the basics now...
SetPhaseVelocity, Do we really need this? The material should now its wave number (eps_r and mue_r) and e.g. openEMS know its target/center frequency? That said it would be better if the EM-tool would just calculate this? But it would also be fine to keep it.
The main source I used to start this off was the review article I referenced. The methods were categorized with the order of the derivatives and the number of phase velocities used. I'm pretty sure (although only from experimentation) it has no effect, so I'll remove this, alltogether. SIBCs somewhat different, so it's best to keep it without the PV paramater
Pyhton (and Matlab) are only interfaces/wrapper around CSXCAD which is a C++ library. Thus all important functionality needs to be implemented in C++ and not in python!
Believe me, that is the case. For the second planned PR there will be a lot of weird stuff with the python, but that is solely to preserve old functionality. All important functionality is kept in C++, and I did some nice tricks to avoid code redundancy (which I dislike, to say the least).
Hey @thliebig, I'd love your input on issue of sheet vs. material, I'll figure out the rest I think.
Cheers
Hi @gadiLhv, I gave this some more thought and maybe we keep it much more as you have it at the moment and reserve a "real" aborbing (material) property for a later date.
That said, keep the name "AbsorbingBC" , keep it as a basic Property (not derived from Material) and maybe even keep the type enumeration. But add as 0 "Undefined" or something? Maybe rename it to ABC_Type? (Absorbing-BC)?
Can you (if possible) rebase this changes on the current version, such that maybe the new CI build checks etc. are included?
br, Thorsten
Hello again
But add as 0 "Undefined" or something?
I didn't understand this, sorry.
Maybe rename it to ABC_Type? (Absorbing-BC)?
I'm guessing once I understand the previous, I'll figure this one out.
Can you (if possible) rebase this changes on the current version, such that maybe the new CI build checks etc. are included?
I pulled from the master thread. You just want me to rebase against the latest commit? Will do.
I'm guessing once I understand the previous, I'll figure this one out.
cpdef enum BCtype "CSPropAbsorbingBC::BCtype":
I meant this enum, rename it maybe to "ABCtype"? And I though adding an enum for the case this was not set or is undefined. Also you can keep it to be plain integers...
Hey @thliebig
I think I got everything you wanted. I also have an answer to another point you raised.
SetPhaseVelocity, Do we really need this? The material should now its wave number (eps_r and mue_r) and e.g. openEMS know its target/center frequency? That said it would be better if the EM-tool would just calculate this? But it would also be fine to keep it.
I will see how to implement this in the EM tool, for sure. For now, I know that if I set a "default" speed, (C0), this happens. I calculated the approximate phase velocity using the MSL effective $\epsilon_r$ $\epsilon_{eff} = 0.5*(1 + \epsilon_{substrate})$, where $\epsilon_{substrate} = 4.5$ which means $v = \sqrt{\frac{1}{\epsilon_{eff}}} C_0\approx 0.6 C_0$
This is still a very weird PR as the changes contain a huge range of changes already in CSXCAD... I guess this is why this still has conflicts. I think you really need to fetch/pull the latest CSXCAD and then rebase/apply you changes and fix all conflicts. Then you should force-push your changes to this PR I guess.
But I had a quick look at the changes that are indeed new. What I do not like is the normal direction definition. openEMS uses 0,1,2 as x,y,z directions and thus you should use this instead of 1,2,3 which would be very confusing otherwise. But I can understand why you need 1-3, because you use the "-" sign for the direction too. And +0/-0 would not work or make much sense. Thus here is my proposal: Get rid of the "normal" direction completely and let the primitives assigned to this property handle this. Since this is supposed to be a boundary condition only planes (2D-boxes) should be allowed as objects for this property anyhow. And thus the direction with zero size is the normal direction. This way you could even have multiple objects in different directions and all of them would work with there respective direction. Then the only thing you would still need to define is the "sign". But that could be done with a simple boolean option. Maybe something like "SetNormalUp" True/False. Or you could define this as a pos/negative phase velocity? Not sure how much sense that would make... What do you think?
What I do not like is the normal direction definition. openEMS uses 0,1,2 as x,y,z directions and thus you should use this instead of 1,2,3 which would be very confusing otherwise. But I can understand why you need 1-3, because you use the "-" sign for the direction too. And +0/-0 would not work or make much sense.
I had a feeling this would be an issue, yet powered through, because of the rest of the work.
Get rid of the "normal" direction completely and let the primitives assigned to this property handle this. Since this is supposed to be a boundary condition only planes (2D-boxes) should be allowed as objects for this property anyhow. And thus the direction with zero size is the normal direction. This way you could even have multiple objects in different directions and all of them would work with there respective direction.
The silly thing is that this 2D only verification already exists in the code. I will modify accordingly.
I think you really need to fetch/pull the latest CSXCAD and then rebase/apply you changes and fix all conflicts. Then you should force-push your changes to this PR I guess.
I'll finish the required changes, and do the rebase\force push thing. The only conflict I saw was one of the conditions I added. Don't know why it was flagged as a conflict...
Sorry for the wait, @thliebig. I'm trying to rebase the whole thing (I am definitely not a professional Git user). Once I get it done, I'll let you know.
Ok @thliebig, I think I got it right this time. I also debugged it so it can actually display the absorbing sheets in the CSXCAD GUI Tested and ready. Once this is merged I'll proceed to the openEMS PR.
This is weird, I'm getting a workflow violation, but can't recreate it on my side. I pulled the latest master, and overwritten my CSXCAD repo over it. Compiles Ok and runs...
Any ideas?
Sorry @thliebig, but there is something wrong with the CI workflow. At the beginning it was trying to pull fparser from my github, instead of yours. I forked fparser to my repo, and now it's giving some error about VTK.
Please let me know what else I can do about this.
Cheers
Hey @thliebig. Any inputs on this? I want to move on to the openEMS changes before I move on to the waveguide modes here.
Cheers
Ok, I started fixing everything. I still need a few inputs before finishing, especially regarding the read\write XML stuff.
Please go ahead with the openEMS part as I would like to review it running together.
Wish I could. Although CSXCAD is a standalone software, the changes I make to openEMS are dependant on it. The good new is, at least locally, I am updating them together, to see that everything is working. It also has it's own, standalone test-script.
As for the "waveguide modes" I would prefer that in a different/new pull request? This way you could in theory already start with that too if you like?
It will have to be a different PR. I am barely handling the github stuff as it is right now.
Excellent. Once this is pushed, I can start the waveguide modes additions in CSXCAD.
Since I don't usually use the XML files, I found this bug rather late. Fixed the boolean read\write for the normal direciton attribute.
Hey @thliebig Any more requirements for this? Pardon me for mixing, but same for PR167 in openEMS.
Ehm ok thanks for the reminder. I think I looked at your changes after the last comment but was not sure if you still work on this or think this is ready for review. I hope I will find time to look at it soon.
Pull 52 here and pull 167 in openEMS are ready for review. The newer ones, not so much...
