-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Incorrect bounds checking on conductive_aux.rho_* #350
Comments
Hi Peter - Sam and I are taking a look at this today. We've managed to get TDMS to run without an error (and without editing the C++/executable code) after applying your suggested change to output-on-main-with-ifdtd-patch.zip This output the result of running the "development version" of TDMS - it includes the partial switch from the MATLAB reader to the HDF5 reader. It seems that these changes (which aren't in version 1.0.1) at least prevent the segmentation fault (likely because we're no longer relying on MATLAB-allocated memory) - but as I mentioned we don't know if it actually fixes the bug or just prevents the seg-fault and produces an incorrect output! |
Hey @prmunro, We travelled back in time to the very first version of the code you gave us in Jan 2022, and we think the above output file is consistent with what was being done back then. If the above file is wrong, could you give us an example pair of input/output files that you are confident in the output's correctness, please? |
Hi @samcunliffe , sorry for the delay in responding. I'll have a look at the development version, however, the main problem with v 1.0.1, or at least the initial problem, is that it is generating an error when dat.conductive_aux.rho_x and dat.conductive_aux.rho_y are matrices. Are you able to see in v 1.0.1 that this is occuring? |
Hi @prmunro,
Yes we are able to see this occurring in version 1.0.1 on our machines too. We also have an idea as to why this bug/error occurs, given the fact that the "development" version is working and 1.0.1 is not:
|
Hi @willGraham01 , my understanding is that TDMS should not be expecting a 50-by-1 vector. I think in the first version of the code, rho_x could be, for exampl, 5-by-10, and TDMS was also expecting that. Please tell me if I'm missing your point above :-) |
Hi Peter - Sam & I came to this conclusion as well; apologies my explanation above wasn't super clear. The short version is: we (incorrectly) added a check to The long version: TDMS originally read the (say Flash forward to our first steps refactoring / reorganisation of the code. Now on the "development version" of TDMS, this bug is not seen because we use the HDF5 reader functions. In particular, we replaced the individual "read vector/matrix/tensor" etc methods with a general "read input data and convert to a flattened array". As such, whatever shape |
Thanks for the above, sorry it's taking me ages to get to this. Can you please remind me how to access the development version of TDMS? |
Hi Peter, The development version of TDMS is the latest version on git clone [email protected]:UCL/TDMS.git # Don't need these two commands if you've already
cd TDMS # got the TDMS repository cloned from GitHub, just
# cd into the appropriate folder
git checkout main
git pull you'll have the development version of the code, which you can then compile with the usual instructions on the website. You can also download the code for the development version as a zip file from github, but the clone method is recommended if you've used the GitHub clone before. |
Hi @willGraham01 , sorry it's taking me a long time to get to this. I just noticed that test arc_15 tests this functionality, so I'm confused as to how the test was passing. Could you please check that this test was indeed passing on the stable version of TDMS? This test can be used to check the correctness of the development version too. By the way, do we test for the creation of the .mat input file? Just asking as when I first noticed this problem, iteratefdtd_matrix.m would generate an error when multilayer was non-empty. So test arc_15 should also generate an error...unless I'm missing something! Peter |
Hi @prmunro,
It looks like
We do when running the system tests (those linked to above). The first stage of these tests is to take the If you have |
Hi @willGraham01 , thanks for this. Here is a onedrive link to arc_14 and arc_15. I hope you can follow the logic of run_pstd_bscan.m. It generates the input .mat files (if recalc is set to 1) and then performs a test against analytically calculated fields. This test code uses an input .m file that is not compatible with TDMS, i.e., it's using my original file format. I know you have a way to deal with this, but let me know if that's a problem. Cheers - Peter |
Hi @prmunro - I've just run these examples through the development version of TDMS. A mixture of good news and bad news with these tests as detailed below. (Applies to both arc_14 and arc_15). For both Attempting to generate So a couple of things here:
|
HI @willGraham01 , thanks for looking in to this. The PSTD algorithm should be used, so we should have use_pstd = 1 and compactsource = 1. However, this also means we need to set hfname = ''. There is also another thing to be mindful of, in particular, efield_gauss contains the following lines: dz = 1300e-9/4; these lines should not be there for TDMS, as TDMS takes care of the -dz/2 term. All of this makes sense in light of your comments. I'm unsure about the best way to proceed. Should I generate input files that should work with TDMS and provide the output from my original version, albeit generated using an old .m input format? Does that make sense? ps: You're very good at being able to work out what's going on here given the intricacy of the code and that you are focused on other projects! Peter |
Hi @prmunro,
Yes I think this is the way to go if we want to debug the original "rho_x and rho_y are not vectors". If you can produce an input Doing this, we should observe that:
From there, we can then work towards releasing a new TDMS version with a fix from the development branch, or identify the root cause of the bug!
I'd missed this - thanks for pointing it out. Looks like MATLAB was picking up this version of |
Hi @willGraham01 , I have created a TDMS compatible test, which could can download here: Running run_pstd_bscan.mwill perform a simulation and compare it with a field calculated analytically. This will tell us if the code is working. Cheers - Peter |
Hi @prmunro - apologies, the .zip file you shared is missing the Regardless;
(Also the |
Hi @willGraham01 , I have generated another zip file, not with efield_gauss_check.m included. I also removed out/pstd_fs.mat as this should be generated by running_pstd_bscan.m and then tdms itself. It was empty because I ran tdms without letting it finish. Can you please try running it again? Peter |
Hi @prmunro, Thanks - script is now running and the development TDMS version reports the error values at the bottom of this post. (If I'm reading the script correctly, these are %-relative errors being reported, but correct me if that's wrong). With this in mind, it looks like the fields are very much out of agreement with the reference data (relative error 1 translates to errors of the same order as the absolute values themselves?). The fact that the relative error is so close to 1 makes me wonder if we're still hitting the In terms of absolute values; the field values themselves have a maximum (absolute) difference around 2, average difference of the order 0.1, and in places a minimum difference of Next StepsI think the next step is to take the input file
Error values reported for development version of TDMS.
|
Hi @willGraham01 , I found some time over Easter to look at this. I found that I had made a mistake in the analysis script, and that the analytic comparison was incorrect. I have included the expected output of erran and errnum in run_pstd_bscan.m (which are absolute and not percentages). Can you see if the development version of TDMS returns the same values? |
Hi @prmunro, the development version of TDMS still returns the values I quoted above: erran =
0.977754538612314 0.977360297407077 0.952807222709342
errnum =
0.0835853336901602 0.0854787661542632 0.156599207597277
0.131404750763419 0.132037636655904 0.146028239606973 even when using the new version of %>> erran
%
%erran =
%
% 0.0798 0.0809 0.3320
%
%>> errnum
%
%errnum =
%
% 0.0535 0.0536 0.3155 0.3414 0.3409 0.0619 (I noticed the zip file you've attached doesn't seem to change the |
Thanks @willGraham01 , the erran relies on reference data calculated analytically within run_pstd_bscan.m, which now calls efield_guass_ml.m. So I am a bit suspicious that the development version is returning identical values for errnum, compared to when we were calling efield_gauss.m. Would you mind double checking that efield_gauss_ml.m is being called? Thanks |
Hi @prmunro, I can confirm Ean = efield_gauss_ml(X,Y,Z);
erran = zeros(1,3);
fields = {'Ex_i','Ey_i','Ez_i','Hx_i','Hy_i','Hz_i'};
for i=1:3
com = sprintf('testfield=dat_test.%s;',fields{i});
eval(com);
erran(i) = sum(sqrt(abs(squeeze(Ean{i})-squeeze(testfield(indsx,find(dat_test.y_i==0),indsz))).^2))/sum(sqrt(abs(squeeze(Ean{i})).^2));
end The numbers remain the same as when I have pushed the $ cd /path/to/TDMS/repo
$ pwd
/path/to/TDMS/repo
$ ls -a
.git doc examples tdms README.md <other top-level files>
$ git checkout main
$ git pull
$ git switch 350-investigation This will fetch the development branch from github and checkout your local repository to that state (you can use |
Describe the bug
When the multilayer variable in the input file is not set to [], there are two errors, one within iteratefdtd_matrix.m and one within the TDMS executeable. The first error (iteratefdtd_matrix) is due to a check to ensure the entries in multilayer increase monotomically, however, this test was specified incorrectly. The second error assumes that resulting data structures (conductive_aux.rho_x and conductive_aux.rho_y) should be vectors, which is incorrect.
To Reproduce
line 299 of iteratefdtd_matrix.m, currently has:
whereas is should be
Inspecting the mat file produced by iteratefdtd_matrix reveals:
meaning that rho_x and rho_y are not vectors, which is the expected behavior. Looking at the original version of the PSTD/FDTD code, we see the following error checking:
which does not require elements to be vectors. I am happy to not check the dimension of these variables since they should be set correctly in iteratefdtd_matrix
input_file_ml.zip
This bug is platform independent.
The text was updated successfully, but these errors were encountered: