search

pyOpenSci / software-submission

Submit your package for review by pyOpenSci here! If you have questions please post them here: https://pyopensci.discourse.group/
94 stars 35 forks source link

Fluidimage submission #194

Open paugier opened 5 months ago

paugier commented 5 months ago

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

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

Technical checks

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

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 - [x] The package has an **obvious research application** according to JOSS's definition in their [submission requirements][JossSubmissionRequirements]. 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. - [x] The package is not a "minor utility" as defined by JOSS's [submission requirements][JossSubmissionRequirements]: "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][JossPaperRequirements] with a high-level description in the package root or in `inst/`. - [x] 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.

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.

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 commented 5 months ago

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.

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.

cmarmo commented 3 months ago

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 commented 1 month ago

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

Thanks a lot for your patience! :pray:

cmarmo commented 1 month ago

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

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

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.

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 commented 1 month ago

@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 commented 1 month ago

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

cmarmo commented 1 month ago

Thank you Leah! And welcome @emmylia321! :wave:

paugier commented 2 weeks ago

@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 commented 2 weeks ago

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 commented 5 days ago

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

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'

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 commented 5 days ago

@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 commented 5 days ago

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.

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 commented 5 days ago

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!