search

FiniteVolumeTransportPhenomena / PyFVTool

Finite volume toolbox in Python
GNU Lesser General Public License v2.1
13 stars 4 forks source link

Renaming of some functions and classes, minor code reorganization and polishing #32

Closed mhvwerts closed 6 months ago

mhvwerts commented 6 months ago

While reading the internal PyFVTool module code in anticipation of work on #15, I started renaming some functions and classes in the hope to better describe their purpose. Also, I reorganized the code a bit, with some polishing and some docstrings. This is only cosmetic: all calculations still work the same.

I still have some further suggestions for renaming, and will continue to add commits to this PR, but please let me already know if the proposed changes are OK for you and you agree with the general approach. It is probably easier to review the proposed changes by one instead of all at once.

I have included a mechanism for maintaining backward compatibility for those changes that also affect the user API side. Personally, I would be in favor of not maintaining such backward compatibility at this early stage, as it is easy for the (few?) users to fix their code to reflect the proposed changes.

mhvwerts commented 6 months ago

@simulkade I only just now discovered your remarks to individual commits. Thanks. I will continue to occasionally update the Mesh objects during my ongoing vacation. It is more work than I initially thought, but I learn quite some new things and I admire how a solid and logical basis you created with the initial PyFVTool. It is easy to follow and reorganize.

simulkade commented 6 months ago

Thanks, @mhvwerts . I am following the reorganization with interest, and go through them as soon as new commits come in. I must admit the code is getting cleaner and more stylish 👍

mhvwerts commented 6 months ago

Yes, more stylish, and it still works!

We should add more testing for 2D and especially for 3D, though. When restyling the 3D meshes, I introduced a bug which was only caught by the PyFVTool-introduction-demo.ipynb Notebook...

BTW, to fix the bug, I had to do something a bit less stylish. It had to do with the handling of multiple dispatch, which is not one of Python strong points. Perhaps move to Julia for this. ;-)

Well, everything seems to work now. Code passes all tests.

mhvwerts commented 6 months ago

I think I have finished for this round of restyling. I have an idea for adapting solvePDE such that the user scripts become more readable, but I will create a separate pull request for that, once this PR will have been accepted.

In src/pyfvtool/__init__.py, I have left ENABLE_LEGACY = False. This will break all existing user scripts, but it is perhaps a good thing to move on, and do the transition at this early stage. We can leave the conditional imports, just in case.

simulkade commented 6 months ago

I have been following the development in this branch and have reviewed all of them. I also think it is a good idea to merge them now. The breaking changes are also not too big of a deal since they are only mesh (and boundary) creation functions that are called perhaps once or twice in each project.