Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

various minor code documentation changes #27

Closed
akaptano opened this issue Oct 27, 2021 · 1 comment
Closed

various minor code documentation changes #27

akaptano opened this issue Oct 27, 2021 · 1 comment

Comments

@akaptano
Copy link

akaptano commented Oct 27, 2021

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
Copy link
Collaborator

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).
    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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants