Kevin-Mattheus-Moerman commented 2 years ago

@Muhao-Chen Below are my review comments for https://github.com/openjournals/joss-reviews/issues/3390

[x] In setup.m the function library path is not added properly due to use of capital L (an issue on Linux), i.e. it should read:

addpath( genpath('Function_library') );

[x] Check the names of functions/examples e.g. main_tower_dyanmic_simple.m should likely be renamed to main_tower_dynamic_simple.m
[x] The use of clear all; has been simplified to clear; in modern MATLAB version
[x] Consider reviewing the code for bad MATLAB style e.g. not allocating arrays and having them grow in for loops. Further more where possible for-loops should be avoided to speed up performance. Example of a for-loop without allocation (from main_tower_dyanmic_simple.m), here of N, MATLAB flags this with a warning too:
```
for i=1:p               % nodal coordinate matrix N
N(:,i)=R*[cos(2*pi*(i-1)/p),sin(2*pi*(i-1)/p),0];
end
```
It is better practice to use something like:
```
N=zeros(p,3); %Allocate N array
for i=1:p               % nodal coordinate matrix N
N(:,i)=R*[cos(2*pi*(i-1)/p),sin(2*pi*(i-1)/p),0];
end
```
Furthermore upon inspection the loop can be avoided to speed up the code since the array can be create "in one go":
```
t=2*pi*((1:p)-1)./p;
N=R*[cos(t); sin(t); zeros(1,p)];
```
[x] Another example of code style (which MATLAB also flags with warnings) is the use of [1:3] instead of the proper (1:3)

[x] It is recommended to use code "indenting", a simple way to do this is to execute CTRL-A (select all) followed by CTRL-I (indent all), i.e. this would transform:

if auto_dt
dt=pi/(8*max(omega));   % time step dt is 1/8 of the smallest period, guarantee convergence in solving ODE
end

to

if auto_dt
dt=pi/(8*max(omega));   % time step dt is 1/8 of the smallest period, guarantee convergence in solving ODE
end

[x] Is the use of global variables strictly speaking needed (e.g. in ode4_tsg.m)? Performance can likely be improved if they can be avoided.
[x] The function ode4_tsg.m features disp(ti); which produces ti on the command window. Without further information this is just a number that streams on the command window (and slows performance). If you really like to display this information I suggest/recommend that you display a comment on what this variable is too. E.g. if you labelled this as Current time: one could use (note also I used sprintf with the format '%.5f' to directly specify the output format and number of decimal places):
```
disp(['Current time: ',sprintf('%.5f',pi)]);
```
Also I suggest/recommend adding an option to not output this number (e.g. implement something like "silentMode"). Note this is just a suggestion that I would look for if I used this code. If the intention is to provide an indication of "completion", then another suggestion/improvement would be to actually compute the % completion and to show that, and one could use waitbar rather than dumping content in the command window.
[x] In the function tenseg_video.m the function tenseg_plot.m is called. The latter appears to be designed to receive a figure handle as one of its inputs. However, within tenseg_video.m it simply passes the number 99, instead of a figure handle.
```
tenseg_plot(N,C_b,C_s,99,[],[],[],R3Ddata);
```
It seems the authors assume the user does not use more than 98 figures and that 99 should be a new figure (In this demo this creates figures 1 up to 10 and a new figure 99). Note however if figure 99 exists this code would override. It would be better practice (and less confusing) to simply open a new empty figure and to pass the figure handle properly. E.g. it looks like the following would do the trick (if one has 10 figures already open this would create an figure numbered 11 rather than 99):
```
hf=figure;
tenseg_plot(N,C_b,C_s,hf,[],[],[],R3Ddata);
```
[x] I get the following error upon running main_10seg_truss_simple.m
```
Error using wgifc
Can only append to GIF89a format GIFs.
```

Error in writegif (line 306) wgifc(mat, map, filename,writemode,disposalmethod,delaytime,...

Error in imwrite (line 566) feval(fmt_s.write, data, map, filename, paramPairs{:});

Error in tenseg_savegif_forever (line 49) imwrite(f,map,filename,'writemode','append','delaytime',dt);

Error in tenseg_video (line 43) tenseg_savegif_forever(name);

Error in main_10seg_truss_simple (line 172) tenseg_video(n_t,C_b,C_s,[0,10.5,-1.5,1.2,0-1,1],100,name,savevideo);



- [x] In `Main_Double_Pendulum_Analytical.m` the parameter `'RelTol'` is initiated as `1e-16`, however I immediately get a MATLAB warning stating: `Warning: RelTol has been increased to 2.22045e-14`. Please check the code and that all parameters (across all example) are indeed suitable. 
- [x] The files to run for the dynamic examples feature both `main_...` and `Main_...`, please choose one type and stick with this consistently. I also suggest renaming the static examples which currently start with `example_...`, perhaps use either `main_...` or `example_...` for all example files. 
- [x] In `Example_folding_3d_Dbar.m` (and several other examples) have an odd comment `%ºÉÔØ×Ó²½`, is this intentional? 
- [x] I get the same gif related error described above in: `Example_lander.m` (line 157)
- [x] In `main_Jasen_linkage_2.m` (and several other static examples) several figure handles show up on the command window as `ans=...`, in other words perhaps a semi-colon `;` is missing to suppress these. 
- [x] For most examples it seems the majority of time is spent writing the visualisation videos/gifs, perhaps consider having the saving be optional so the demos run fast and could skip this export. 
- [x] In my personal project [GIBBON](https://github.com/gibbonCode/GIBBON) I use a MATLAB script called `testGibbon` (https://github.com/gibbonCode/GIBBON/blob/master/lib/testGibbon.m) to run all tests/demos automatically. Consider adopting a similar automated testing scheme. Running all automatically (or even manually) would be simplified if all examples could be found in a single folder but this is not required.

Kevin-Mattheus-Moerman commented 2 years ago

@Muhao-Chen let me know if you have any questions. Happy to help explain more.

Kevin-Mattheus-Moerman commented 2 years ago

@Muhao-Chen are you able to address these issues?

diehlpk commented 2 years ago

@Muhao-Chen Please start working on these remarks, so we can proceed with the review.

diehlpk commented 2 years ago

@Kevin-Mattheus-Moerman It seems that @Muhao-Chen has addressed some of your comments. Can you please have a look if he resolved the issues?

Muhao-Chen commented 2 years ago

@Muhao-Chen Below are my review comments for openjournals/joss-reviews#3390

[x] In setup.m the function library path is not added properly due to the use of capital L (an issue on Linux), i.e. it should read:
addpath( genpath('Function_library') );
Thank you for pointing it out! We have changed this part as you suggested.

[x] Check the names of functions/examples e.g. main_tower_dyanmic_simple.m should likely be renamed to main_tower_dynamic_simple.m Thank you for pointing it out! We have changed the name of the examples as you suggested.

[x] The use of clear all; has been simplified to clear; in the modern MATLAB version Thank you for pointing it out! We have changed all the 'clear all' to 'clear' as you suggested.

[x] Consider reviewing the code for bad MATLAB style e.g. not allocating arrays and having them grow in for loops. Furthermore where possible for-loops should be avoided to speed up performance. Example of a for-loop without allocation (from main_tower_dyanmic_simple.m), here of N, MATLAB flags this with a warning too:
for i=1:p               % nodal coordinate matrix N
    N(:,i)=R*[cos(2*pi*(i-1)/p),sin(2*pi*(i-1)/p),0];
end
It is better to practice using something like:
N=zeros(p,3); %Allocate N array
for i=1:p               % nodal coordinate matrix N
    N(:,i)=R*[cos(2*pi*(i-1)/p),sin(2*pi*(i-1)/p),0];
end
Furthermore upon inspection the loop can be avoided to speed up the code since the array can be created "in one go":
t=2*pi*((1:p)-1)./p;
N=R*[cos(t); sin(t); zeros(1,p)];
Thanks for your suggestion! This part is changed as you suggested.

[x] Another example of code style (which MATLAB also flags with warnings) is the use of [1:3] instead of the proper (1:3) Thanks for your suggestion. We have revised this part.

[x] It is recommended to use code "indenting", a simple way to do this is to execute CTRL-A (select all) followed by CTRL-I (indent all), i.e. this would transform:
if auto_dt
dt=pi/(8*max(omega));     % time step dt is 1/8 of the smallest period, guarantee convergence in solving ODE
end
to
if auto_dt
    dt=pi/(8*max(omega));     % time step dt is 1/8 of the smallest period, guarantee convergence in solving ODE
end
Thanks for your suggestion! Automatic indenting is used for all the examples.

[x] Is the use of global variables strictly speaking needed (e.g. in ode4_tsg.m)? Performance can likely be improved if it can be avoided. Thanks for pointing it out! We do agree that performance can be improved if global variables can be avoided. The speed is acceptable at this moment, since the simulation doesn't take long even for very complex examples in our tests. But considering future research problems, i.e., integrating structure design, control, and optimization problems, this is a very good direction to make the performance even better.

[x] The function ode4_tsg.m features disp(ti); which produces ti on the command window. Without further information, this is just a number that streams on the command window (and slows performance). If you really like to display this information I suggest/recommend that you display comment on what this variable is too. E.g. if you labeled this as Current time: one could use (note also I used sprintf with the format '%.5f' to directly specify the output format and a number of decimal places):
disp(['Current time: ',sprintf('%.5f',pi)]);
Also I suggest/recommend adding an option to not output this number (e.g. implement something like "silentMode"). Note this is just a suggestion that I would look for if I used this code. If the intention is to provide an indication of "completion", then another suggestion/improvement would be to actually compute the % completion and to show that, and one could use waitbar rather than dumping content in the command window.

[x] In the function tenseg_video.m the function tenseg_plot.m is called. The latter appears to be designed to receive a figure handle as one of its inputs. However, within tenseg_video.m it simply passes the number 99, instead of a figure handle.
tenseg_plot(N,C_b,C_s,99,[],[],[],R3Ddata);
It seems the authors assume the user does not use more than 98 figures and that 99 should be a new figure (In this demo this creates figures 1 up to 10 and a new figure 99). Note however if figure 99 exists this code would override. It would be better practice (and less confusing) to simply open a new empty figure and to pass the figure handle properly. E.g. it looks like the following would do the trick (if one has 10 figures already open this would create a figure numbered 11 rather than 99):
hf=figure;
tenseg_plot(N,C_b,C_s,hf,[],[],[],R3Ddata);
Thanks for your comment. This part is revised as you suggested.

[x] I get the following error upon running main_10seg_truss_simple.m
Error using wgifc
Can only append to GIF89a format GIFs.

Error in writegif (line 306)
wgifc(mat, map,
filename,writemode,disposalmethod,delaytime,...

Error in imwrite (line 566)
        feval(fmt_s.write, data, map, filename,
        paramPairs{:});

Error in tenseg_savegif_forever (line 49)
    imwrite(f,map,filename,'writemode','append','delaytime',dt);

Error in tenseg_video (line 43)
        tenseg_savegif_forever(name);

Error in main_10seg_truss_simple (line 172)
tenseg_video(n_t,C_b,C_s,[0,10.5,-1.5,1.2,0-1,1],100,name,savevideo); 
[x] In Main_Double_Pendulum_Analytical.m the parameter 'RelTol' is initiated as 1e-16, however I immediately get a MATLAB warning stating: Warning: RelTol has been increased to 2.22045e-14. Please check the code and that all parameters (across all examples) are indeed suitable. Thanks for pointing it out. This warning is due to the computational errors by matlab. This warning can be neglected by 'warning off'. From our test examples, the results are accurate.

[x] The files to run for the dynamic examples feature both main_... and Main_..., please choose one type and stick with this consistently. I also suggest renaming the static examples which currently start with example_..., perhaps use either main_... or example_... for all example files. Thanks for pointing it out. We have changed all the file names to 'Main_...' as you suggested.

[x] In Example_folding_3d_Dbar.m (and several other examples) have an odd comment %ºÉÔØ×Ó²½, is this intentional? Thanks for pointing it out. That is language errors. We have changed the comments to English.

[x] I get the same gif related error described above in: Example_lander.m (line 157)

[x] In main_Jasen_linkage_2.m (and several other static examples) several figures handle show up on the command window as ans=..., in other words perhaps a semi-colon ; is missing to suppress these. Thanks for your suggestion. semi-colon; is added.

[x] For most examples it seems the majority of time is spent writing the visualization videos/gifs, perhaps consider having the saving be optional so the demos run fast and could skip this export. Thank you for your comments. The default has been changed to not save video and data.

[x] In my personal project GIBBON I use a MATLAB script called testGibbon (https://github.com/gibbonCode/GIBBON/blob/master/lib/testGibbon.m) to run all tests/demos automatically. Consider adopting a similar automated testing scheme. Running all automatically (or even manually) would be simplified if all examples could be found in a single folder but this is not required. Thank you so much for your suggestion! We will check and learn from your valuable experience. Thanks!

diehlpk commented 2 years ago

@Kevin-Mattheus-Moerman I think @Muhao-Chen worked on some of your remarks. Could you please have a look?

diehlpk commented 2 years ago

@Kevin-Mattheus-Moerman I think @Muhao-Chen worked on some of your remarks. Could you please have a look?

Kevin-Mattheus-Moerman commented 2 years ago

[x] In setup.m, the following old syntax is still used producing a warning:
```
clear all;
```
The new syntax is simply:
```
clear; 
```
[x] In ode4_tsg.m and perhaps other codes/functions, code indentation is still not used, see comments above to fix this e.g.:
```
if silentMode==0&useWaitbar==1
wb= waitbar(0,'Please wait...');
end
```
[x] Also, check the above snippet e.g. if silentMode==0&useWaitbar==1 should read silentMode==0 && useWaitbar==1
[x] The file Main_tower_static.m features a capital M in Main while other codes have all lower case main. Consider making this consistent across all file names. Also check Main_half_Tbar_static.m, as well as all static demos e.g. Main_lander.m.
[x] I get the following error in main_Jasen_linkage_1.m and similarly in main_Jasen_linkage_2.m, main_Jasen_linkage_3.m, and main_cable_dome.m:
```
Unrecognized function or variable 'tf'.
```

Error in main_Jasen_linkage_1 (line 151) name=['Jasenmachanism','tf',num2str(tf),material{1}];


- [x] Some codes (e.g. `main_Jasen_linkage_1.m`) still implement `figure(99);`. Implement the fix suggested above for all codes. 
- [x] Various codes contain stuff like `ºÉÔØ×Ó²½` (e.g. `main_Jasen_linkage_1.m`). Are these perhaps language characters my computer does not know? Please review all codes and ensure all content works for an any international contributors. 
```matlab
xlabel('ºÉÔØ×Ó²½','fontsize',14);

this produces the error:

Warning: Error updating Text.

 String scalar or character vector must have valid interpreter
 syntax:
ºÉÔØ×Ó²½

[x] Please use ; (e.g. main_Jasen_linkage_1.m) to terminate statements with outputs you do not which to have on the command screen currently. In tenseg_plot_catenary is used without a ; causing the figure handle to be produced in the command window. Add semi-colons to suppress these. Check this in all codes.
[x] The automated testing suggested has still not been added.
[x] The code still feature non-meaningful or unspecified command window markers. Add words to say what these are where possible so the user knows what is been shown e.g.
```
disp(['Current time: ',sprintf('%.5f',pi)]);
```
[x] In tenseg_ex_force.m the input a is not used, one should remove it if not needed.
[x] In tenseg_ex_force.m the switch statement says 'impluse', this should like be fixed to 'impulse'
[x] In tenseg_ex_force.m one could use tspan<0.05 instead of find(tspan<0.05) to save time, i.e. there is not need to use find.
[x] In tenseg_ex_force.m there is not point to the empty array allocation dz_a_t=[];
[x] In tenseg_ex_force.m, for the 'vib_force' option, the variables dz_d_t, dz_v_t, and dz_a_t, are not used.
[x] In main_lander.m check spelling of dynaimcs

[x] In main_lander.m, none of the outputs of the following line (124) appear to be used:

[w_t,dnb_t,dnb_d_t,dnb_dd_t,dz_a_t]=tenseg_ex_force(tspan,a,b,'impluse',gravity,[0;0;9.8],C,mass,3*(4:9)-1,0,period);

[x] Question(s): The animation for the main_lander.m example appears to feature "contact" of the structure with a surface. Can you clarify how this is implemented? Is there documentation covering this type of boundary condition?
[x] Also review the previously unchecked boxes above, and check occurrences of the points I make in other codes. Thanks.

Kevin-Mattheus-Moerman commented 2 years ago

@Muhao-Chen @diehlpk :wave: see above. I went through the changes. Some more changes needed.

Muhao-Chen commented 2 years ago

@Kevin-Mattheus-Moerman @diehlpk Thank you so much for all the valuable suggestions. We followed your advice and made changes. Please have a check and let me know if you have any comments. Thank you!!

I would also like to respond to some of the pointed issues.

[x] Various codes contain stuff like ºÉÔØ×Ó²½ (e.g. main_Jasen_linkage_1.m). Are these perhaps language characters my computer does not know? Please review all codes and ensure all content works for any international contributors. Thank you for pointing it out! Yes, we have international collaborators. So we provided both English and Chinese descriptions. Because the language setting of your Windows operating system is not in Chinese, so you see the wield codes. To solve this issue, we provided both language options for users. As you may see, in the help description of function ansys_input_gp.m, there is a language option: language=0; % 0 for English,1 for Chinese. For elsewhere, we have changed all the descriptions into English.

[x] The automated testing suggested has still not been added. Yes, we have followed your provided procedure and added this function. Very cool and thank you!

[x] Question(s): The animation for the main_lander.m example appears to feature "contact" of the structure with a surface. Can you clarify how this is implemented? Is there documentation covering this type of boundary condition? The ground is modeled as a second-order system with ground stiffness and damping. The function and description are given at here: Tensegrity_Finite_Element_Method_TsgFEM/Software_Verification_and_Examples/Dynamics_Examples/04_Lander/w_t_ground_force.m.

diehlpk commented 2 years ago

@Kevin-Mattheus-Moerman Could you please have a look?

diehlpk commented 2 years ago

@Kevin-Mattheus-Moerman Could you please have a look?

diehlpk commented 2 years ago

@Kevin-Mattheus-Moerman Could you please have a look?

Kevin-Mattheus-Moerman commented 2 years ago

Most things are now fixed, please review and consider merging this pull request to fix remaining issues: https://github.com/Muhao-Chen/Tensegrity_Finite_Element_Method_TsgFEM/pull/5

Muhao-Chen commented 2 years ago

@Kevin-Mattheus-Moerman Thank you so much for your help! I have merged the pull request! Thank you, indeed!

Kevin-Mattheus-Moerman commented 2 years ago

@Muhao-Chen

[x] I created one more pull request for you to consider: https://github.com/Muhao-Chen/Tensegrity_Finite_Element_Method_TsgFEM/pull/6
[x] The function ansys_input_gp_00 still contains what looks like non-english symbols my system cannot parse. Please update. See line 45 m£¬N£¬Pa£¬s, line 149 !Êä³ö½Úµã×ø±ê£¬ÍØÆË¹ØÏµ£¬½ØÃæ»ý£¬Ô¤Ó¦Á¦, and lines 155-158 e.g. ¨Òå²ÄÁÏÌØÐÔ(Êý¾Ý), line 163 !½Úµã×ø±ê ....and many others.
[x] functions like tenseg_mode and tenseg_video_strain_stress save images to the current working directory. Change this throughout to be a local folder. Also using git to track these creates a mess. It would be best to output all images to a temporary folder and to gitignore that folder.
[x] tenseg_video_strain_stress and tenseg_ex_force, and perhaps others, have unused variables e.g. pic_num, and a. Please check and update codes accordingly.
[x] It would be nice if the source for the manual was shared too so people can help edit that too.

diehlpk commented 2 years ago

@Muhao-Chen Please work on these comments soon.

diehlpk commented 2 years ago

@Muhao-Chen Please work on these comments soon.

Muhao-Chen commented 2 years ago

[x] The function ansys_input_gp_00 still contains what looks like non-english symbols my system cannot parse. Please update Thanks for your question. There is a Language option in the code, so one can choose to use English or Chinese. As you may see in ansys_input_gp_00 , line 27, language=0; % 0 for English,1 for Chinese.

[x] functions like tenseg_mode and tenseg_video_strain_stress save images to the current working directory. Change this throughout to be a local folder. Also using git to track these creates a mess. It would be best to output all images to a temporary folder and to gitignore that folder. Thank you! We have followed your valuable suggestion and added the data_temp folder and used the gitignore.

[x] tenseg_video_strain_stress and tenseg_ex_force, and perhaps others, have unused variables e.g. pic_num, and a. Please check and update codes accordingly. Thank you for pointing this out. Yes, we have some unused variables, some of them are left because they may be used for different simulation cases and future development. For example, one wants to simulate an earthquake or impulse to the structure, so he/she can use the unused variables and modify the loading conditions a little bit. Thank you!

[x] It would be nice if the source for the manual was shared too so people can help edit that too. Thank you! We have uploaded the word source file.

Muhao-Chen commented 2 years ago

@Kevin-Mattheus-Moerman @diehlpk Please have a check and let me know if you have any questions anytime! Thank you so much for your great help in improving the codes!

diehlpk commented 2 years ago

@Kevin-Mattheus-Moerman Could you please have a look, I would like to finish the paper soon.

Kevin-Mattheus-Moerman commented 2 years ago

@Muhao-Chen

[x] The function tenseg_video_strain_stress still saves a file locally (outside of data_temp folder). If you made name an input to the function then the codes calling this function can specify the save path to point to data_temp.
[x] Your .gitignore file is not working properly, you could change it to be like below. Note the change relating to the data_temp folders:
```
# Ignore MATLAB temporary files
*.asv
```

Ignore temp file for running tests

*.mat

Ignore gif animations files

*.gif

Ignore png images

*.png

Ignore txt files

*.txt

Ignore folder content

**/data_temp

Specifically I removed the comment symbol `#` and added ** so that any data_temp folder is ignore (you are using multiple). Note that you are currently ignoring all txt, png, and gif files. This also means that changes in the Videos folder and other images relating to documentation may not be tracked. You may consider perhaps if you want this or if these are no longer needed after ignoring those data_temp files. 

- [x] Recommendation: The Jansen codes contain `machanism`, which should read `mechanism`
- [x] Recommendation: The code `main_Double_Pendulum_Analytical.m` uses uppercase letters while `main_double_pendulum_simple` uses lowercase letters. I recommend lower case throughout (or to at least have them be the same). 
- [x] Unfortunately it is not very repeatable but I now get this type of error every now and then when running the tests:

Error using matlab.internal.imagesci.wgifc Can only append to GIF89a format GIFs.

Error in writegif (line 306) matlab.internal.imagesci.wgifc(mat, map, filename,writemode,disposalmethod,delaytime,...

Error in imwrite (line 566) feval(fmt_s.write, data, map, filename, paramPairs{:});

Error in tenseg_savegif_forever (line 56) imwrite(f,map,filename,'writemode','append','delaytime',dt);

Error in tenseg_video_slack (line 45) tenseg_savegif_forever(name);

Error in main_Jansen_linkage_1 (line 152) tenseg_video_slack(n_t,C_b,C_s,l0_t,index_s,[],[0,90],[-80,80,-85,50,0,70],min(substep,30),name,savevideo,material{2})

Muhao-Chen commented 2 years ago

@Kevin-Mattheus-Moerman Thanks for the comments. All the issues have been fixed. Please have a check, and thank you again!

Kevin-Mattheus-Moerman commented 2 years ago

@Muhao-Chen thanks for making those changes. That last error :point_up: still occurs some times but again it is not repeatable, could be a linux graphics issue, I am not sure, often if I rerun the code it doesn't happen so it is odd. One last issue, when running the dynamics tests using auto_dynamics_test, I got an error as MATLAB could not find main_tbar_dynamics. This code is currently called main_Tbar_dynamics (note the capital T). To fix it you need to rename it, or you have to change the code in auto_dynamics_test to have a capital T.

If you fix this last point I believe all my issues are addressed.

Muhao-Chen commented 2 years ago

@Kevin-Mattheus-Moerman Thanks for your help! Yeah, probably, the error does show up on my windows environment. I have renamed main_Tbar_dynamics.m as main_tbar_dynamics.m. Thank you so much!

@diehlpk Please also have a check on everything. Thank you for your time and effort!

diehlpk commented 2 years ago

@Muhao-Chen I will have a check once @Kevin-Mattheus-Moerman checked all boxes in the main review thread.

Muhao-Chen / Tensegrity_Finite_Element_Method_TsgFEM

JOSS review general comments #3

Ignore temp file for running tests

Ignore gif animations files

Ignore png images

Ignore txt files

Ignore folder content