search

mdolab / CMPLXFOIL

GNU General Public License v2.0
29 stars 20 forks source link

py3 fixes #1

Closed ewu63 closed 3 years ago

ewu63 commented 3 years ago

Purpose

I have no idea why I never submitted this PR... but anyhow here it is. I don't remember if this works or not so reviewers should check this. We should also at least add some Azure build jobs to make sure this thing builds.

Type of change

What types of change is it? Select the appropriate type(s) that describe this PR

Testing

Explain the steps needed to test the new code to verify that it does indeed address the issue and produce the expected behavior.

Checklist

Put an x in the boxes that apply.

ewu63 commented 3 years ago

I know this is not set up as a package or anything, I think that should probably be done as a separate PR just to keep things separate.

bernardopacini commented 3 years ago

This looks good to me. It compiles and runs on my Mac with gfortran without issues.

One question: did you update and generate the changes to the src_cs/ files in the PR automatically via the complexify script?

ewu63 commented 3 years ago

No I actually did not, and actually I'm not sure if the complexify.py script is ever used. Those fixes were done manually---please check and make sure that they make sense, I'm not super familiar with Fortran.

bernardopacini commented 3 years ago

Interesting. The changes seem correct, but I will try the complexify.py script to see if it is useful / we can automatic the complexify process.

bernardopacini commented 3 years ago

I tried out the complexify script and it does not quite work (the generated code appears reasonable but does not compile). Also, it is Python2 instead of Python3.

I propose that we merge this PR as is, since it compiles and runs, and then I can work on the complexify process so that it is automated. Does this sound reasonable @nwu63 @Xiaosong2105?

ewu63 commented 3 years ago

I propose that we merge this PR as is, since it compiles and runs, and then I can work on the complexify process so that it is automated. Does this sound reasonable @nwu63 @Xiaosong2105?

I am fine with merging this now, but I would say that getting the complexify process automated is probably very low priority, since the repo is stable and the code will likely not change much. I would say turning this into a Python package (with setup.py etc.) and then setting up CI/CD is probably more important. Then there are docs and tutorials and so on that could be improved.

bernardopacini commented 3 years ago

I am fine with merging this now, but I would say that getting the complexify process automated is probably very low priority, since the repo is stable and the code will likely not change much. I would say turning this into a Python package (with setup.py etc.) and then setting up CI/CD is probably more important. Then there are docs and tutorials and so on that could be improved.

That sounds good to me. I am not very familiar with the internals but I imagine XFoil itself is still largely the same as is implemented here. Getting the package install and CI/CD should be quick, I'll take care of those later this week.

Xiaosong2105 commented 3 years ago

I propose that we merge this PR as is, since it compiles and runs, and then I can work on the complexify process so that it is automated. Does this sound reasonable @nwu63 @Xiaosong2105?

I am fine with merging this now, but I would say that getting the complexify process automated is probably very low priority, since the repo is stable and the code will likely not change much. I would say turning this into a Python package (with setup.py etc.) and then setting up CI/CD is probably more important. Then there are docs and tutorials and so on that could be improved.

Sure, this sounds good to me. I will also look deeper into this code after merging this PR. @nwu63 @bernardopacini