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

Fluidimage submission #194

Open
18 of 28 tasks
paugier opened this issue May 30, 2024 · 18 comments
Open
18 of 28 tasks

Fluidimage submission #194

paugier opened this issue May 30, 2024 · 18 comments
Assignees

Comments

@paugier
Copy link

paugier commented May 30, 2024

Submitting Author: Pierre Augier (@paugier)
All current maintainers: (@paugier)
Package Name: Fluidimage
One-Line Description of Package: "Fluidimage: a Python framework to study flows from images"
Repository Link: https://foss.heptapod.net/fluiddyn/fluidimage
Version submitted: 0.5.3
EIC: @Batalex
Editor: @cmarmo
Reviewer 1: @leahmendelson
Reviewer 2: @stefanv
Reviewer 3: @emmylia321
Archive: TBD
JOSS DOI: TBD
Version accepted: TBD
Date accepted (month/day/year): TBD


Code of Conduct & Commitment to Maintain Package

Description

  • Include a brief paragraph describing what your package does:

FluidImage is a free and open-source Python framework to process images of fluids (in particular with PIV), and analyse the resulting fields.

Fluidimage has now grown into a clean software reimplementing in modern Python algorithms and ideas taken from UVmat, OpenPIV, PIVlab and PIVmat with a focus on performance, usability and maintanability. However, Fluidimage is not restricted to Particle Image Velocimetry computations (PIV, i.e. displacements of pattern obtained by correlations of cropped images) and can be used to (i) display and pre-process images, (ii) compute displacement or velocity fields with PIV, Background-Oriented Schlieren (BOS) and optical flow, and (iii) analyze and display vector and scalar fields.

Scope

  • Please indicate which category or categories.
    Check out our package scope page to learn more about our
    scope. (If you are unsure of which category you fit, we suggest you make a pre-submission inquiry):

    • Data retrieval
    • Data extraction
    • Data processing/munging
    • Data deposition
    • Data validation and testing
    • Data visualization[^1]
    • Workflow automation
    • Citation management and bibliometrics
    • Scientific software wrappers
    • Database interoperability
  • For all submissions, explain how and why the package falls under the categories you indicated above. In your explanation, please address the following points (briefly, 1-2 sentences for each):

    • Who is the target audience and what are scientific applications of this package?

      The target audience is students, scientists, researchers and engineers working with images of fluids. Fluidimage can be used to process images, compute vectors and scalar fields and analyze them.

    • Are there other Python packages that accomplish the same thing? If so, how does yours differ?

      OpenPIV can be used to compute vectors with various PIV algorithms. Fluidimage is not restricted to PIV. It has a very different API than OpenPIV. Fluidimage has more focus on performance, with in particular compiled extensions produced with Transonic-Pythran and an asynchronous/parallel framework to describe "topologies" of computations and compute then in parallel.

Technical checks

For details about the pyOpenSci packaging requirements, see our packaging guide. Confirm each of the following by checking the box. This package:

  • does not violate the Terms of Service of any service it interacts with.
  • uses an OSI approved license.
  • contains a README with instructions for installing the development version.
  • includes documentation with examples for all functions.
  • contains a tutorial with examples of its essential functions and uses.
  • has a test suite.
  • has continuous integration setup, such as GitHub Actions CircleCI, and/or others.

Publication Options

I tend to think that Fluidimage is not yet ready for a paper in JOSS. We need more time to get a stronger community and to implement few more features before submitting to JOSS.

JOSS Checks
  • The package has an obvious research application according to JOSS's definition in their submission requirements. Be aware that completing the pyOpenSci review process does not guarantee acceptance to JOSS. Be sure to read their submission requirements (linked above) if you are interested in submitting to JOSS.
  • The package is not a "minor utility" as defined by JOSS's submission requirements: "Minor ‘utility’ packages, including ‘thin’ API clients, are not acceptable." pyOpenSci welcomes these packages under "Data Retrieval", but JOSS has slightly different criteria.
  • The package contains a paper.md matching JOSS's requirements with a high-level description in the package root or in inst/.
  • The package is deposited in a long-term repository with the DOI:

Note: JOSS accepts our review as theirs. You will NOT need to go through another full review. JOSS will only review your paper.md file. Be sure to link to this pyOpenSci issue when a JOSS issue is opened for your package. Also be sure to tell the JOSS editor that this is a pyOpenSci reviewed package once you reach this step.

Are you OK with Reviewers Submitting Issues and/or pull requests to your Repo Directly?

This option will allow reviewers to open smaller issues that can then be linked to PR's rather than submitting a more dense text based review. It will also allow you to demonstrate addressing the issue via PR links.

  • Yes I am OK with reviewers submitting requested changes as issues to my repo. Reviewers will then link to the issues in their submitted review.

Note that Fluidimage development is done on https://foss.heptapod.net/fluiddyn/fluidimage and that issues have to be filled on this website. Pull requests are of course very welcome. Note that with Heptapod/Mercurial, one should not fork the repo to submit a PR (see https://fluiddyn.readthedocs.io/en/latest/mercurial_heptapod.html).

Confirm each of the following by checking the box.

  • I have read the author guide.
  • I expect to maintain this package for at least 2 years and can help find a replacement for the maintainer (team) if needed.

Please fill out our survey

P.S. Have feedback/comments about our review process? Leave a comment here

Editor and Review Templates

The editor template can be found here.

The review template can be found here.

@Batalex
Copy link
Contributor

Batalex commented Jun 22, 2024

Editor in Chief checks

Hi there! Thank you for submitting your package for pyOpenSci review. Below are the basic checks that your package needs to pass to begin our review. If some of these are missing, we will ask you to work on them before the review process begins.

Please check our Python packaging guide for more information on the elements
below.

  • Installation The package can be installed from a community repository such as PyPI (preferred), and/or a community channel on conda (e.g. conda-forge, bioconda).
    • The package imports properly into a standard Python environment import package.
  • Fit The package meets criteria for fit and overlap.
  • Documentation The package has sufficient online documentation to allow us to evaluate package function and scope without installing the package. This includes:
    • User-facing documentation that overviews how to install and start using the package.
    • Short tutorials that help a user understand how to use the package and what it can do for them.
    • API documentation (documentation for your code's functions, classes, methods and attributes): this includes clearly written docstrings with variables defined using a standard docstring format.
  • Core GitHub repository Files
    • README The package has a README.md file with clear explanation of what the package does, instructions on how to install it, and a link to development instructions.
    • Contributing File The package has a CONTRIBUTING.md file that details how to install and contribute to the package.
    • Code of Conduct The package has a CODE_OF_CONDUCT.md file.
      There is a dedicated section on the website, I think we're good to go.
    • License The package has an OSI approved license.
      NOTE: We prefer that you have development instructions in your documentation too.
  • Issue Submission Documentation All of the information is filled out in the YAML header of the issue (located at the top of the issue template).
  • Automated tests Package has a testing suite and is tested via a Continuous Integration service.
  • Repository The repository link resolves correctly.
  • Package overlap The package doesn't entirely overlap with the functionality of other packages that have already been submitted to pyOpenSci.
  • Archive (JOSS only, may be post-review): The repository DOI resolves correctly.
  • Version (JOSS only, may be post-review): Does the release version given match the GitHub release (v1.0.0)?

  • Initial onboarding survey was filled out
    We appreciate each maintainer of the package filling out this survey individually. 🙌
    Thank you authors in advance for setting aside five to ten minutes to do this. It truly helps our organization. 🙌


Editor comments

Hello @paugier, and welcome to pyOpenSci! Sorry it took me so long to get back to you, I have been sitting on these checks for a while now.
fluidimage is truly an interesting project, even from a code structure perspective. I never used mercurial, and I think it's a first for pyOpenSci in general. Same for the licence, never encountered it before!
I'll get started on finding an editor. Thank you for your patience.

@Batalex Batalex self-assigned this Jun 22, 2024
@Batalex Batalex moved this from under-review to seeking-editor in peer-review-status Jun 22, 2024
@cmarmo
Copy link
Member

cmarmo commented Aug 13, 2024

Hello @paugier I'm Chiara, following up as editor in chief during those summer months.
I just want to let you know that you haven't been forgotten ... hope to find an editor for Fluidimage soon 🤞.

@cmarmo cmarmo assigned cmarmo and unassigned Batalex Oct 1, 2024
@lwasser lwasser moved this from seeking-editor to under-review in peer-review-status Oct 1, 2024
@cmarmo
Copy link
Member

cmarmo commented Oct 1, 2024

Hello @paugier we are very sorry for making you wait so long!
If you are still interested in our review process (I hope so! 😇), I'm starting looking for reviewers for your submission.

Thanks a lot for your patience! 🙏

@cmarmo
Copy link
Member

cmarmo commented Oct 20, 2024

Hello @paugier , I am glad to announce that we have two reviewers for Fluidimage. 🎉

@leahmendelson and @stefanv kindly accepted to take care of this submission: thank you so much to you both! 🙏

Leah, Stefan, before beginning your review, please fill out our pre-review survey. This helps us improve all aspects of our review and better understand our community. No personal data will be shared from this survey - it will only be used in an aggregated format by our Executive Director to improve our processes and programs.

  • reviewer 1 survey completed.
  • reviewer 2 survey completed.

The following resources will help you complete your review:

  1. Here is the reviewers guide. This guide contains all of the steps and information needed to complete your review.
  2. Here is the review template that you will need to fill out and submit here as a comment, once your review is complete.

Please get in touch with any questions or concerns!
This review went already a bit out of schedule, I hope a three weeks deadline is ok for you both.
Otherwise please let me know, so we can keep track.

This review is then due on November 10th.

Thank you so much again to you both and to Pierre for his patience!

Happy review to all of you!

@cmarmo
Copy link
Member

cmarmo commented Oct 20, 2024

@leahmendelson, I almost forgot.... Do you mind confirming the github handle of your student, so I can add them to the review acknowledgment? Thank you!

@leahmendelson
Copy link

@emmylia321 is the student who will be co-reviewing with me

@cmarmo
Copy link
Member

cmarmo commented Oct 22, 2024

Thank you Leah! And welcome @emmylia321! 👋

@paugier
Copy link
Author

paugier commented Nov 7, 2024

@emmylia321, @leahmendelson and @stefanv, just to tell you that I've just released a new version of fluidimage (0.5.4) with a small fix of a bug with a new version of Matplotlib. There are also wheels for Python 3.13, but in practice they should not be used before scikit-image 0.25 is really released (only 0.25.0rc1 is available right now).

@cmarmo
Copy link
Member

cmarmo commented Nov 10, 2024

Hello dear reviewers, just checking if you had time to move forward with review.
Please let us know! If you need more time I totally understand, I just need to have an insight on the schedule.
@emmylia321, @leahmendelson and @stefanv , thank you for your collaboration!

@stefanv
Copy link

stefanv commented Nov 20, 2024

Hi @paugier! Thanks for your submission. Today, I worked on installing the package from source, and here's an initial set of notes I took while doing so:


The README has a broken link to https://en.wikipedia.org/wiki/Particle_image_velocimetry_(PIV)
Same for docs frontpage.

CeCILL license is, of course fine and choice of the developers, but worth noting that it is more restrictive than the BSD license used by most of the scientific Python ecosystem.

Package minimal dependency for scikit-image is very old; consider updating? See, e.g., SPEC0

Like with the license, Mercurial is fully the developers' choice, but may exclude significant part of ecosystem developers (I used to use Mercurial, but have no idea how to anymore 🤷‍♂️).

Consider shipping example images in separate repository—they really blow up the repo size. See, e.g. pooch.

Consider adding an INSTALL.md or similar with build-from-source instructions; can simply be a link to: https://fluidimage.readthedocs.io/en/latest/build-from-source.html.
Many developers will clone repo and look there for how to proceed.

The installation instructions suggest that pdm install --no-self would create a new virtual env, but instead:

$ pdm install --no-self

INFO: Inside an active virtualenv /home/stefan/envs/py312, reusing it.
Set env var PDM_IGNORE_ACTIVE_VENV to ignore it.

When compiling, lots of Pythran warnings, may be worth reporting to Serge:

  src/fluidimage/calcul/interpolate/__pythran__/thin_plate_spline.cpp:288:13: warning: possibly dangling reference to a temporary [-Wdangling-reference]

skimage now has thin plate splines, btw: https://scikit-image.org/docs/stable/auto_examples/transform/plot_tps_deformation.html
Not sure if that implementation is useful or good enough for this application, but we'd love feedback :)

make tests resulted in a traceback with:

ModuleNotFoundError: No module named 'coverage.results'
  • Upgrading from coverage 7.6.4 to 7.6.7 worked.
  • Next, psutil was not found. Huh, something wrong with my psutil install? Reinstalling it fixed the problem.
  • Next ERROR src/fluidimage - AttributeError: module 'cv2' has no attribute 'imread'.
  • Reinstalling opencv-python did the trick. OK, at this point I'm thinking pdm may have done something to mess up my environment.
  • Also reinstalled pillow, scipy, kiwisolver, pyyaml to fix import errors.

Yay, test suite passed! Some warnings to resolve:

src/fluidimage/gui/test_monitor.py: 4 warnings
src/fluidimage/test_run_from_xml.py: 4 warnings
src/fluidimage/topologies/test_bos.py: 2 warnings
src/fluidimage/topologies/test_example.py: 11 warnings
src/fluidimage/topologies/test_image2image.py: 10 warnings
src/fluidimage/topologies/test_mean.py: 6 warnings
src/fluidimage/topologies/test_optical_flow.py: 8 warnings
src/fluidimage/topologies/test_piv.py: 12 warnings
src/fluidimage/topologies/test_preproc.py: 8 warnings
  /usr/lib64/python3.12/multiprocessing/popen_fork.py:66: DeprecationWarning: This process (pid=891926) is multi-threaded, use of fork() may lead to deadlocks in the child.
    self.pid = os.fork()

Will pick up from here soon!

@paugier
Copy link
Author

paugier commented Nov 20, 2024

@stefanv thanks for looking at this. I'm going to answer in more details and fix things, but it is clear that PDM did not do what it should do in particular because you run it from a virtual env used for something else.

I guess things are going to work much better if you don't do that. PDM should create a dedicated environment in /.../fluidimage/.venv

@paugier
Copy link
Author

paugier commented Nov 20, 2024

Thanks @stefanv for your notes. I worked on a merge request to improve the situation (https://foss.heptapod.net/fluiddyn/fluidimage/-/merge_requests/116) and opened two issues related to two of your remarks.

The README has a broken link to https://en.wikipedia.org/wiki/Particle_image_velocimetry_(PIV) Same for docs frontpage.

Fixed in https://foss.heptapod.net/fluiddyn/fluidimage/-/merge_requests/116/diffs?commit_id=eaa510beea8938743010f6ef2add5fd2a9e230be

CeCILL license is, of course fine and choice of the developers, but worth noting that it is more restrictive than the BSD license used by most of the scientific Python ecosystem.

CeCILL is equivalent (or better in terms of European laws) than GPL. I wonder how it would be possible than users of a software like fluidimage could be restricted in practice by GPL. However, I asked to the developers if they agree to change to BSD.

Package minimal dependency for scikit-image is very old; consider updating? See, e.g., SPEC0

I updated few minimal dependencies in https://foss.heptapod.net/fluiddyn/fluidimage/-/merge_requests/116/diffs?commit_id=17347a3dca48900011283f2a868b177490d83e52

Like with the license, Mercurial is fully the developers' choice, but may exclude significant part of ecosystem developers (I used to use Mercurial, but have no idea how to anymore 🤷‍♂️).

I have to admit that I doubt that Mercurial could be a barrier for anyone really wanting to contribute to Fluidimage.

  • for beginners, Mercurial is still simpler than Git
  • for people used to Git, we have documentation (accessible in particular from our file CONTRIBUTING.md) and our workflow is really simple.

I tend to think that people knowing Git will be able after 2 minutes of reading to run

hg clone ...
cd fluidimage
hg topic my-great-topic-name
hg status
hg commit -m "..."
hg push

More generally, I really think that too much uniformity in tech is bad. For version control, the Git quasi monopoly for open-source is bad. For some aspects, Git is nice, but for some others, it is really bad and old. For example, it is really unfortunate that people cannot safely share history edition and need to push with --force or --force-with-lease. Also the need to use a lot git commit -a is really unfortunate.

Therefore, I consider that it is part of my research tasks to work on one of the alternatives for version control. Mercurial, still used internally at Meta (actually a fork), Google (only the client) and Mozilla, is interesting.

Consider shipping example images in separate repository—they really blow up the repo size. See, e.g. pooch.

This is indeed interesting. A newly cloned repo (cloned in ~ 5 s so it is still fine) has a size of a bit less than 100MB, which is too much. I opened an issue on this https://foss.heptapod.net/fluiddyn/fluidimage/-/issues/42

Consider adding an INSTALL.md or similar with build-from-source instructions; can simply be a link to: https://fluidimage.readthedocs.io/en/latest/build-from-source.html. Many developers will clone repo and look there for how to proceed.

Done in https://foss.heptapod.net/fluiddyn/fluidimage/-/merge_requests/116/diffs?commit_id=677bd1af0f23fa800eac9466a803a629cdf7cb38

The installation instructions suggest that pdm install --no-self would create a new virtual env, but instead:

$ pdm install --no-self

INFO: Inside an active virtualenv /home/stefan/envs/py312, reusing it.
Set env var PDM_IGNORE_ACTIVE_VENV to ignore it.

This is really unfortunate that you experienced such problems! Especially because we use good modern tools and everything works well for us with PDM (and also Pixi).

I updated our documentation to avoid your install issue in https://foss.heptapod.net/fluiddyn/fluidimage/-/merge_requests/116/diffs?commit_id=08b63f2c392d4c5a1e2999f527b2531929cbc568

When compiling, lots of Pythran warnings, may be worth reporting to Serge:

With a virtual env correctly setup with PDM, we also have few warnings related to Pythran, but not the same as you! Anyway, I'm going to report this in https://github.com/serge-sans-paille/pythran/

skimage now has thin plate splines, btw: https://scikit-image.org/docs/stable/auto_examples/transform/plot_tps_deformation.html Not sure if that implementation is useful or good enough for this application, but we'd love feedback :)

I'm going to look at that.

make tests resulted in a traceback with:

Yay, test suite passed! Some warnings to resolve:

All these issues are related to your broken environment. With a correct environment created with PDM, Fluidimage tests pass without warning.

@stefanv
Copy link

stefanv commented Nov 20, 2024

All these issues are related to your broken environment. With a correct environment created with PDM, Fluidimage tests pass without warning.

Great, thanks @paugier! I will rebuild a dedicated virtual environment and re-test. The new documentation says:

You should not run the following commands from a virtual environment not related to Fluidimage.

I think a lot of people start from an existing environment, and after decades of using virtual envs of never tried to get out of one! There is no deactivate command, so not sure how to do that. Looks like pdm --venv .venv or similar may do the trick, though.

Re: the hg and license comments, those were merely observations / data points; totally fine for the project to make opinionated choices, of course. I understand the motivation for libre licenses; one practical issue in our ecosystem is that code cannot flow from GPL packages into BSD libraries. E.g., say you had a good thin-plate splines implementation that we wanted to adopt into scikit-image. But, I'm not pushing you to change the license.

I want to make very clear that my goal with this review is to help the project as much as I can, and to provide some ecosystem context where useful. None of the comments are meant as criticism!

@cmarmo
Copy link
Member

cmarmo commented Dec 7, 2024

Hello @leahmendelson @emmylia321 @stefanv , I wonder if we can have the reviews in before Christmas?
Please let me know if you need any help / info.

Thank you so much for volunteering your time for pyOpenSci.

@cmarmo
Copy link
Member

cmarmo commented Jan 11, 2025

Hello everybody and happy new year to all of you!
I hope you enjoyed some holiday break, we are now back to business and I wondering if we can set a timeline for the reviews.
@leahmendelson @emmylia321 @stefanv do you mind letting me know if you are able to finalize your review?
Otherwise I can also look for someone else... everyone is busy and this is a volunteer job, so I totally understand if you have issues in moving forward. Please just let us know so we can take the necessary actions.

Thank you so much for your work!

@emmylia321
Copy link

emmylia321 commented Jan 14, 2025 via email

@leahmendelson
Copy link

Sorry for the delays - we should be able to close this out within this and next week.

@leahmendelson
Copy link

Package Review (Still underway, but wanted to get the checklist going)

Please check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide

  • As the reviewer I confirm that there are no conflicts of interest for me to review this work (If you are unsure whether you are in conflict, please speak to your editor before starting your review).

Documentation

The package includes all the following forms of documentation:

  • A statement of need clearly stating problems the software is designed to solve and its target audience in README.
  • Installation instructions: for the development version of the package and any non-standard dependencies in README.
  • Vignette(s) demonstrating major functionality that runs successfully locally.
  • Function Documentation: for all user-facing functions.
  • Examples for all user-facing functions.
  • Community guidelines including contribution guidelines in the README or CONTRIBUTING.
  • Metadata including author(s), author e-mail(s), a url, and any other relevant metadata e.g., in a pyproject.toml file or elsewhere.

Readme file requirements
The package meets the readme requirements below:

  • Package has a README.md file in the root directory.

The README should include, from top to bottom:

  • The package name
  • Badges for:
    • Continuous integration and test coverage,
    • Docs building (if you have a documentation website),
    • A repostatus.org badge,
    • Python versions supported,
    • Current package version (on PyPI / Conda).

NOTE: If the README has many more badges, you might want to consider using a table for badges: see this example. Such a table should be more wide than high. (Note that the a badge for pyOpenSci peer-review will be provided upon acceptance.)

  • Short description of package goals.
  • Package installation instructions
  • Any additional setup required to use the package (authentication tokens, etc.)
  • Descriptive links to all vignettes. If the package is small, there may only be a need for one vignette which could be placed in the README.md file.
    • Brief demonstration of package usage (as it makes sense - links to vignettes could also suffice here if package description is clear)
  • Link to your documentation website.
  • If applicable, how the package compares to other similar packages and/or how it relates to other packages in the scientific ecosystem.
  • Citation information

Usability

Reviewers are encouraged to submit suggestions (or pull requests) that will improve the usability of the package as a whole.
Package structure should follow general community best-practices. In general please consider whether:

  • Package documentation is clear and easy to find and use.
  • The need for the package is clear
  • All functions have documentation and associated examples for use
  • The package is easy to install

Functionality

  • Installation: Installation succeeds as documented.
  • Functionality: Any functional claims of the software been confirmed.
  • Performance: Any performance claims of the software been confirmed.
  • Automated tests:
    • All tests pass on the reviewer's local machine for the package version submitted by the author. Ideally this should be a tagged version making it easy for reviewers to install.
    • Tests cover essential functions of the package and a reasonable range of inputs and conditions.
  • Continuous Integration: Has continuous integration setup (We suggest using Github actions but any CI platform is acceptable for review)
  • Packaging guidelines: The package conforms to the pyOpenSci packaging guidelines.
    A few notable highlights to look at:
    • Package supports modern versions of Python and not End of life versions.
    • Code format is standard throughout package and follows PEP 8 guidelines (CI tests for linting pass)

Final approval (post-review)

  • The author has responded to my review and made changes to my satisfaction. I recommend approving this package.

Estimated hours spent reviewing:


Review Comments (Partial list to keep things moving while I work on the rest)

  • Missing repostatus.org badge
  • Pre-compiled pip install succeeded (Windows 11, Python 3.9), still need to test build from source
  • Tutorials and examples using get_path_image_samples() all failed when using the pre-compiled install. Maybe something in the precomplied paths didn't generalize? There was limited documentation for me to debug anything that calls this function.
  • Will pick up from here soon!

@emmylia321 also had the following notes (running on macOS)

  • Tutorial could be slightly more explicit in parts, though it is generally understandable: E.g. midpage it said, “Let’s go!”, I was not sure if this was a celebration or an instruction to run the file.
  • Got the following error, even though this variable was correctly set to 1 using the instructed code: “SystemError: For performance reason,the environment variable OMP_NUM_THREADS has to be set to "1" before executing a Fluidimage topology.” Fixed this by setting the variable directly in the terminal using: export OMP_NUM_THREADS=1
  • Couldn't display the final field displays, fixed this with result.display(show_correl=False, hist=True. in the script, which makes sense, but it could've been clearer in the tutorial

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: under-review
Development

No branches or pull requests

6 participants