Submit pyflct for review as an Affiliated Package

nabobalis commented 4 years ago

Package Details

Package name: pyflct
Maintainer(s): No one
Repository: https://github.com/sunpy/pyflct
PyPI link (if applicable): https://pypi.org/project/pyflct/

Description of Package

A Python wrapper for Fourier Local Correlation Tracking.

Package Review

Editor Submission Checklist

[x] Is the submission appropriate?
[x] Does the project have an appropriate license?
[x] Is the project in a version control repository?
[x] Does the project provide a Python interface?
[x] Is the project on PyPI?
[x] Is the project useful to the solar physics community?

Instructions to Reviewer

Please copy the following and select the ranking for each criteria, the full review criteria can be found here:

* Functionality           : General Package / Specialized Package / Not Relevant
* Integration             : Well integrated / Partially Integrated / Minimal Integration
* Documentation           : Extensive / Some / Little
* Testing                 : Excellent / Good / Needs Work
* Duplication             : None / Some / Major
* Community               : Excellent / Good / Needs Work
* Development Status      : Stable / Rapid Development / Low Activity / Needs Work

Cadair commented 4 years ago

@ayshih would you be able to review this?

ayshih commented 4 years ago

Will do

ayshih commented 4 years ago

Functionality: Specialized Package This package is useful for those specific folks who need to perform Fourier local correlation tracking (FLCT).
Integration: No Integration There is not yet any integration into the SunPy-specific ecosystem, with no use of any applicable functionality (e.g., ndcube).
Documentation: Some The documentation (https://pyflct.readthedocs.io) is not linked from the repository. The API appears to be well documented, but the documentation in general is fairly rough, with little guidance for users. The first gallery example ought to be elevated to primary documentation. As a detail, I found the description of its licenses to be confusing.
Testing: Excellent Tests appear to be thorough.
Duplication: None None noted.
Community: Good There isn't a history of community interaction for this package in specific, but the developers are actively involved with the community.
Development Status: Needs Work While the package is functional, the health of development is unclear.

Cadair commented 4 years ago

There are no versioned releases yet.

this is no longer accurate as of last night. https://pypi.org/project/pyflct/0.1/ :grinning:

That said, it's not clear where SunPy functionality would be appropriate.

This is interesting, and potentially a bug in the review criteria. In my mind this isn't a reason for a package to not get a passing grade. For instance drms has no real dependence on the rest of the sunpy ecosystem, but it's obviously still relevant and useful, it just lives at the bottom of the stack. (sunpy core depends on it, not the other way around). Thoughts?

ayshih commented 4 years ago

this is no longer accurate as of last night. https://pypi.org/project/pyflct/0.1/ 😀

Ha! I checked the GitHub repo this morning before I submitted the report, but I think my web browser showed me a cached version. I've updated my review.

This is interesting, and potentially a bug in the review criteria. In my mind this isn't a reason for a package to not get a passing grade. For instance drms has no real dependence on the rest of the sunpy ecosystem, but it's obviously still relevant and useful, it just lives at the bottom of the stack. (sunpy core depends on it, not the other way around). Thoughts?

I agree that the criteria should probably be revised, and I even checked the drms review to see what the reviewer said. Of course, sunpy depends on drms, so it's integrated in that sense. There's no integration either way with pyflct (that I'm aware of).

Cadair commented 4 years ago

There's no integration either way with pyflct (that I'm aware of).

I think this is true. I don't know if @nabobalis has thought about this, but this was originally in sunkit-image (removed for licencing issues) and I think there is scope for scikit-image (or maybe just in pyflct) to add higher level functions, i.e. to take in ndcube / map objects and return velocity cubes.

I wonder what peoples opinions on if this should be red, (i.e. should this package be provisional pending implementation of such functionality), or if this is just an orange?

nabobalis commented 4 years ago

I think this is true. I don't know if @nabobalis has thought about this, but this was originally in sunkit-image (removed for licencing issues) and I think there is scope for scikit-image (or maybe just in pyflct) to add higher level functions, i.e. to take in ndcube / map objects and return velocity cubes.

I've not but it is a good idea to increase integration with sunpy/ndcube.

dstansby commented 4 years ago

I wonder what peoples opinions on if this should be red, (i.e. should this package be provisional pending implementation of such functionality), or if this is just an orange?

I would say red. The main function (https://pyflct.readthedocs.io/en/latest/api/pyflct.flct.html#pyflct.flct) definitely has scope to take two GenericMap and return GenericMap. If it is intended to be general purpose (ie. only work with arrays) then I don't think it's appropriate to be a SunPy affiliated package.

I also note that this has no maintainer listed, and I think having a maintainer should be a criteria for being an affiliated package.

ayshih commented 4 years ago

If it is intended to be general purpose (ie. only work with arrays) then I don't think it's appropriate to be a SunPy affiliated package.

Interesting. So if something like sunkit-image implemented Map-aware FLCT with pyflct as a dependency, pyflct wouldn't be ~rated as well integrated~ considered appropriate to be an affiliated package, but sunkit-image would be?

ayshih commented 4 years ago

I also note that this has no maintainer listed, and I think having a maintainer should be a criteria for being an affiliated package.

I assumed that @nabobalis was the maintainer. If this is truly maintainer-less, I ought to downgrade my rating for "Development Status".

dstansby commented 4 years ago

Interesting. So if something like sunkit-image implemented Map-aware FLCT with pyflct as a dependency, pyflct wouldn't be ~rated as well integrated~ considered appropriate to be an affiliated package, but sunkit-image would be?

I would say yes - as far as I can see pyflct is a general-purpose image processing technique, whereas sunkit-image in the example given would provide the user-facing interface through which it can be applied to solar data.

Cadair commented 4 years ago

If this is truly maintainer-less, I ought to downgrade my rating for "Development Status".

This is under the sunpy org because it's a useful output of a GSOC project, if @nabobalis doesn't want to look after it I will be listed as the maintainer.

Cadair commented 4 years ago

as far as I can see pyflct is a general-purpose image processing technique

Should this exclude it from being sunpy affiliated? I can kinda see both sides of this, it definitely fits in the "functionality" criteria, this is useful to solar physicists. On the other hand maybe it would be better to not implement solar specific functionality in pyflct and it would be better to do that in sunkit-image (as sunkit-image builds on scikit-image it could build on pyflct).

Even if we say we want to keep pyflct as a low level wrapper, it's still useful to solar physicists, and to my mind people would benefit from having it listed on the affiliated package page (not to mention we are maintaining it for this reason).

A practical note (not actually relevant to the theoretical discussions) this is a separate package due to complex licensing issues (BSD, LGPL and GPL dependencies) I took the decision that it would be much easier to have this packaged separately so the separation was clearer. I think in doing this we allow it to be imported into sunkit-image without triggering the GPL linking clause, as that is only a problem when you link against fftw3, which happens at build time in the pyflct wheels.

I am therefore unhappy with the concept that just because this isn't part of sunkit-image it doesn't deserve affiliated package status.

nabobalis commented 4 years ago

If this is truly maintainer-less, I ought to downgrade my rating for "Development Status".

This is under the sunpy org because it's a useful output of a GSOC project, if @nabobalis doesn't want to look after it I will be listed as the maintainer.

I just don't want to keep adding to my list of package that I look after but officially don't maintain. If bug reports or changes need to be made on them, I will make them or accept PRs and do releases. But as things stand I already look after a ton of low priority/volume packages and I keep seeming to increase that list.

Cadair commented 4 years ago

Is your issue that the list is too long or that you don't "officially" look after them?

nabobalis commented 4 years ago

The first one and I also don't want to officially look after them but right now there is no one else.

ayshih commented 4 years ago

Still not relevant to the affiliated-package discussion, but:

I think in doing this we allow it to be imported into sunkit-image without triggering the GPL linking clause, as that is only a problem when you link against fftw3, which happens at build time in the pyflct wheels.

While reasonable people will debate how GPL works with respect to imported Python libraries, I think an appreciable fraction would say that the intent of GPL is that if sunkit-image ever has critical functionality (i.e., the functionality is a fundamental part of the project) that has a hard dependence on importing pyflct (i.e., there is no fallback if pyflct is not present), then sunkit-image must also inherit GPL from pyflct.

One cumbersome workaround is to have the non-GPL code interact with the GPL code through the OS by spawning processes and passing data files. That way the Python interpreter never actually interprets GPL code.

Ideally, pyflct would be LGPL instead of GPL, but it can't be because fftw is GPL. Incidentally, since fftw is GPL, I don't see how LGPL can be a valid license for the Berkeley C code.

ayshih commented 4 years ago

Still still not relevant to the affiliated-package discussion, but:

On the fftw website (http://www.fftw.org/faq/section1.html#isfftwfree), they say that it is possible to purchase a non-free, unlimited-use license. That could be an avenue to relicensing everything under LGPL.

ayshih commented 4 years ago

I created an issue on pyflct's repo to discuss the license: https://github.com/sunpy/pyflct/issues/11

ayshih commented 4 years ago

I revised my report per the updated criteria descriptions.

sunpy / sunpy.org