YingyiLiu / HAMS

An open-source computer program for the analysis of wave diffraction and radiation of three-dimensional floating or submerged structures
Apache License 2.0
100 stars 50 forks source link

Enable compilation with gfortran on linux/mac #2

Closed gbarter closed 3 years ago

gbarter commented 3 years ago

We are interested in using your code and making a python wrapper around the executable. We also want to be able to run this on Macs and our Linux HPC. To enable compilation with gfortran, I had to make some formatting and other minor fixes to variable declarations. Eventually we would like to explore using HAMS as a library, so some of the changes also support use of f2py, although more effort will be required to get that working.

Please review and test this out in your environment and workflow to ensure that I have not introduced any changes that breaks anything on your side.

gbarter commented 3 years ago

I have actually been able to make a direct python driver to HAMS here: https://github.com/WISDEM/pyHAMS Still plenty of work to avoid the input and output files in the driver, but it is a start.

Credit to other NREL colleagues for the hard work on the file preprocessing functions

YingyiLiu commented 3 years ago

Dear Garrett,

Thank you for your interest and also your nice work on making the python wrapper.

I've reviewed all the changes that you have made to the code, and I do not find any problem at all.

I've also run the HAMS_x64.exe file (that you have complied using gfortran and stored in your repository pyhams/pyhams/bin/) with the case DeepCwind, and I also find no problem.

But to my surprise, when I download your version of HAMS code from the repository pyhams/pyhams/src/, and after compiling using Intel Fortran and run with the DeepCwind case, I find there are two problems:

1). In the Excitation_x.rao file, wave headings are output in several lines instead of one line (see the enclosed), mainly due to the change that you have made in SUBROUTINE PrintHeading: WRITE(NFILE, '(A8,(7X,F7.2))') '#HEADING ',(NWVHD(II),II=1,NBETA) to WRITE(NFILE, '(A8,(7X,F7.2))') '#HEADING ',(NWVHD(II),II=1,NBETA)

2). the excitation force output in both WAMIT and Hydrostar format are NaN values while the added mass and the wave dampings are not. I checked the code, it seems that MXPOT(IEL,7,IP) is always zero while MXPOT(IEL,1,IP)~ MXPOT(IEL,6,IP) are not.

Have you made any other changes in the code (pyhams/pyhams/src/) that have not been shown in the 6 commits that I reviewed?

Best regards, Yingyi

Garrett Barter notifications@github.com 于2021年2月14日周日 下午12:21写道:

I have actually been able to make a direct python driver to HAMS here: https://github.com/WISDEM/pyHAMS Still plenty of work to avoid the input and output files in the driver, but it is a start.

Credit to other NREL colleagues for the hard work on the file preprocessing functions

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/YingyiLiu/HAMS/pull/2#issuecomment-778716308, or unsubscribe https://github.com/notifications/unsubscribe-auth/AJ2G3NCIT3GGDVZAKONVIDDS646SBANCNFSM4XSKIRUQ .

gbarter commented 3 years ago

Hello Yingyi,

Thank you for your time and thorough review!

For the Excitation_x.rao file, gfortran does not like variable format expressions: https://gcc.gnu.org/onlinedocs/gfortran/Variable-FORMAT-expressions.html

So, these lines need to be updated accordingly WRITE(NFILE, '(A8,<NBETA>(7X,F7.2))') '#HEADING ',(NWVHD(II),II=1,NBETA) WRITE(NFILE, '(A8,<NBETA>(7X,F7.2))') '#HEADING ',(WVHD(II),II=1,NBETA) In my haste, I just removed the variable formatting, but should really be using the adjustl call, as you have done elsewhere in the code. You are a better Fortran programmer than I am. Would you be able to suggest how these lines can be reworked to remove the formatting?

For the question of NaNs and 0s in MXPOT, could this be an issue with initialization? Should this loop: https://github.com/YingyiLiu/HAMS/blob/master/SourceCode/AssbMatx.f90#L271 and here: https://github.com/YingyiLiu/HAMS/blob/master/SourceCode/AssbMatx_irr.f90#L482 instead be DO 1400 MD=1, 7?

Cheers, Garrett

YingyiLiu commented 3 years ago

Dear Garrett,

Thanks for your quick response.

1) adjustl is for a different purpose. But I think this issue can be solved using a similar way as that in SUBROUTINE PrintBody_CmplxVal by printing the formatting to a variable FMT: WRITE(FMT,*) '(F8.4,',NBETA,'(ES14.6),',NBETA,'(F12.4))' WRITE(NFILE,FMT) W1,(MDL(II),II=1,NBETA),(PHS(II),II=1,NBETA) ! WRITE(NFILE,200) W1,(NREL(II),II=1,NBETA),(NIMG(II),II=1,NBETA) !100 FORMAT(F8.4,(ES14.6),(F12.4)) !200 FORMAT(F8.4,(ES14.6),(ES14.6))

2) The two loops do not matter, as the radiation and the diffraction potentials are solved in separate subroutines. This issue sounds strange to me. I will look into it in detail.

By the way, I've just improved my code this afternoon in my local PC repository which perhaps has solved some of the outputting issues, but these commits have not been submitted to the online repository yet. I am afraid whether my submission of these most recent commits (which is later than your commits) will disable the merging of your commits to my repository or not, as I'm not quite familiar with the Git rules. If it will not, I will submit these commits immediately and you can pull them from the HAMS origin master branch to update your version of code and send me your merge request again. How do you think of that?

Best regards, Yingyi

gbarter commented 3 years ago

Hi Yingyi,

Github is very adept at merging multiple changes and managing conflicts. You should only approve and merge my pull request once we are 100% convinced that it works. In the meantime, you should feel free to continue pushing changes to your master branch in this repository. I can bring in those changes into my fork and my pull request will be automatically updated. For instance, if you fix the NBETA formatting issue on your side, I can incorporate that on my side and then it will disappear as a change.

Regarding the MXPOT, I will also do some digging on my side.

Cheers, Garrett

gbarter commented 3 years ago

I realized I commented out much of the output to the Excitation_* files for the same variable formatting reason. It is possible that once we resolve that everything will fall into place. It sounds like you already had some code improvements in that regard, so I will wait for your changes.

YingyiLiu commented 3 years ago

Dear Garrett,

Thanks. I have submitted my latest updates to the online repository, in which the following changes are included:

1) allows for strange elements with overlapping vertexes in the mesh input. 2) fixes the wave angular frequencies in the Hydrostar motion output file. 3) replaces tabs with spaces in the control files of all cases to prevent NaN outputs 4) fixes the NBETA formatting issue

The MXPOT issue has not been fixed yet. I would like to see that after your updating of pyHAMS whether this issue will disappear. If not, I will fix that separately.

Best regards, Yingyi

Garrett Barter notifications@github.com 于2021年2月17日周三 上午3:13写道:

I realized I commented out much of the output to the Excitation_* files for the same variable formatting reason. It is possible that once we resolve that everything will fall into place. It sounds like you already had some code improvements in that regard, so I will wait for your changes.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/YingyiLiu/HAMS/pull/2#issuecomment-780022981, or unsubscribe https://github.com/notifications/unsubscribe-auth/AJ2G3NF4KOUZ4H7OU6HDWNLS7KYTBANCNFSM4XSKIRUQ .

gbarter commented 3 years ago

Thank you, Yingyi! I have incorporated your changes in my fork of HAMS in this PR and also the pyHAMS directory too. Note that I did find a couple of <NBETA> remaining in PrintOutput.f90, but it looked like you had just forgotten to substitute in the FMT variable so I went ahead and did that.

All changes relative to your code are captured in this PR in the "Files Changed" tab. Please take stock and let me know what differences you see in the output that concern you and I can help track it down.

YingyiLiu commented 3 years ago

Dear Garrett,

That's great! I checked your new merge request and finally found that the MXPOT problem is related to the variable re-declarations in the SUBROUTINE Initialisation (as these variables has been already defined in WaveDyn_mod). The variable re-declarations lead to zero value of the variable AMP outside of SUBROUTINE Initialisation when compiled by Intel Fortran. Therefore after accepting your merge request, I commented out these variable re-declarations in the SUBROUTINE Initialisation and now this problem is resolved.

Thanks very much for your co-working on this issue.

Best regards, Yingyi

Garrett Barter notifications@github.com 于2021年2月17日周三 下午8:57写道:

Thank you, Yingyi! I have incorporated your changes in my fork of HAMS in this PR and also the pyHAMS directory too. Note that I did find a couple of

remaining in PrintOutput.f90, but it looked like you had just forgotten to substitute in the FMT variable so I went ahead and did that. All changes relative to your code are capture in this PR in the "Files Changed" tab. Please take stock and let me know what differences you see in the output that concern you and I can help track in down. — You are receiving this because you commented. Reply to this email directly, view it on GitHub , or unsubscribe .
gbarter commented 3 years ago

Thank you so much, Yingyi! Those re-declarations were definitely my mistake. Thank you for catching them. All looks good on my end too.

YingyiLiu commented 3 years ago

Dear Garrett,

That's OK, because you might not realize that these global variable has already been defined in a separate module.

BTW, I've also posted the links to your two new software in the "Useful links" section on the HAMS page.

Best regards, Yingyi

Garrett Barter notifications@github.com 于2021年2月18日周四 下午8:02写道:

Thank you so much, Yingyi! Those re-declarations were definitely my mistake. Thank you for catching them. All looks good on my end too.

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/YingyiLiu/HAMS/pull/2#issuecomment-781263252, or unsubscribe https://github.com/notifications/unsubscribe-auth/AJ2G3NGAINLML7JKQPZEA7DS7TXULANCNFSM4XSKIRUQ .