CSXCAD icon indicating copy to clipboard operation
CSXCAD copied to clipboard

Tried to add missing CSProperties Python interfaces

Open RFLAH opened this issue 1 year ago • 5 comments

CSProperties Python interface updated for missing features like dispersive materials. This was the best I could achieve. I hope it makes some sense.

RFLAH avatar May 02 '24 21:05 RFLAH

Thanks for your commit, it will surely help drive this forward and I will hopefully have time for a review soon.

One thing I immediately noticed is that you have a wild mixture of spaces and tabs. While this is not a compile issue for C-Code, it is a problem for readability and coding style. Can you please cleanup the tabs and replace them all with 4 spaces? This is true for the python code section. For the C++-Code section ("src"-folder) different rules apply (I think there it is tabs only for indentation).

thliebig avatar May 03 '24 11:05 thliebig

Thank you for your comments Thorsten. I will check the tabs and spaces soon.

RFLAH avatar May 03 '24 12:05 RFLAH

Hi I have tried to compile this, but I get multiple errors about false indentation and mixed use of tabs and spaces.

Have you actually tried to compile and test your code? If not, please do before... There is an extra folder CSXCAD/python/tests with some unit tests for most of the interface functions which you could extend for your new interfaces. But in any case you always need to test your code! At least I have so far never been able to write code that was just working out of the box and on the first attempt... (at least nothing non-trivial)

thliebig avatar May 06 '24 18:05 thliebig

Hi Thorsten,

sorry for the inconvenience. I did remove all the tabs now at least according to search engine, but I have not tried to compile this since I do not have the tools and/or skills for that. This was one of the reasons I was hoping to leave this coding part for pros... I have currently very limited time available for this and it would probably take too much time and effort at the moment. I could not be sure what errors are due to code itself and what due to lack of compiling experience. I will need to take a time out here and get back to this on better time and maybe after some compiling studies and exercises.

I am sure that the code will not run instantly and maybe this PR would have been better to split in smaller portions initially too. I was kind of hoping maybe other more skilled developers could contribute on this PR to get this python interface up to date too. There were some features I was not sure how they were intended to work like unknown property and discrete material for example, but I tried to make a guess. I was not sure about the variable naming in pyx-file either. Are they already defined somewhere in the source code or can they be freely named in pyx-file? Are upper-case variable names ok (e.g. EpsPlasma vs. epsplasma) etc...

Thank you and please do not use too much effort on this PR. It would actually be interesting to try to compile this and see what was wrong or was anything right, but I really have to pass it for now. By the way I used the Github web dev option to create the PR and I noticed something odd happened for the indentations when I copied the code changes to the web dev edit window from my desktop IDE and committed the changes.

RFLAH avatar May 06 '24 21:05 RFLAH

I will and cannot merge broken/not compiling code and I would really prefer that you could fix this and squash and force push this as a single rebased (on the current tree) working and tested! commit. It is not ideal to have commits in a tree that do not even compile nor run if you would ever have to git bisect...

thliebig avatar Dec 26 '24 20:12 thliebig