FlowModelingControl / flowtorch

flowTorch - a Python library for analysis and reduced-order modeling of fluid flows
GNU General Public License v3.0
131 stars 45 forks source link

various minor code documentation changes #27

Closed akaptano closed 2 years ago

akaptano commented 2 years ago

Okay I am done with my review. I will finalize it on the review GitHub page when we resolve the issues I submitted. The code and code documentation looks excellent. Good use of comments and class inheritance. Last optional comments about minor code functionality and documentation:

  1. (Optional) It might be nice to add some documentation in sub functions of the various classes, for instance in functions like "interact" in psp_explorer.py. It looks like you already do this here and there in the code.
  2. (Optional) The code is meant to streamline the post-processing + analysis of fluid flows and does a great job for combining fluid flow data from disparate sources. But other than the large set of file readers, the code and examples have somewhat limited functionality in terms of producing ROMs or performing some statistical analysis -- only the traditional SVD, DMD, CNM, are implemented/illustrated in the examples. My suggestion for future work is to either (1) expand the code with enhanced capabilities, or (2) link to pre-existing packages for advanced analyses, since SVD, DMD, CNM, and other methods all have specific Python packages.
AndreWeiner commented 2 years ago

Hi Alan,

1) I checked all files for missing docstrings and added them where necessary. Sometimes the docstrings of non-private member functions in derived classes are left blank on purpose because, in the Sphinx documentation, the docstring is inherited from the base class if it is not overwritten in the derived class (pretty cool feature of Sphinx). 2) 1) The development of flowtorch is an ongoing process. For the next release, I already have functionality for outlier detection and a robust SVD/DMD variant plus examples in the pipeline. Also, if you look at the class structure of ROM and Encoder, you will find that idea is to add more derived classes of each in the feature such that one can combine all available encoders with all available ROMs (currently, there is only one of each, so the intension is probably not perfectly clear yet). 2) It is indeed possible to use flowtorch in conjunction with existing packages specialized in doing only a single step in the analysis and modeling pipeline. I've been doing that as a matter of fact myself with PyDMD. This possibility is now mentioned in the README, and I also added links to two existing complementary packages.

Feel free to close this issue if you are happy with my answers. Many thanks again for your suggestions!

Andre