aresio / simpful

A friendly python library for fuzzy logic reasoning
Academic Free License v3.0
129 stars 33 forks source link

Comments on the package #1

Closed jonas-eschle closed 4 years ago

jonas-eschle commented 4 years ago

Dear all, I've declined an invitation to review your paper given my lack of knowledge in fuzzy logic. However, having looked at your repository, I may leave some comments here from a Python perspective that you may consider improving on. Given that the main advantage of your package comes from having a package, not just implementing a new algorithm, this aspects seem crucial. I'll noted below the aspects that I would improve on and they are of course subject to my personal and professional experience, which is limited and surely biased. I hope however, that this will help you and I do not want to discourage you from developing a package, but rather help you by providing a realistic and honest feedback. Depending on your scope, they may also aim too high.

1) The largest part of work in a repository is NOT the creation of it, having a bunch of scripts up and running, but the maintenance. The work after the publication. Adding features, fixing bugs and so on. And in this sense I see a significant lack of things. Let me expand: 1.1) maybe consider moving the repository to an organization. This makes it easier to have multiple maintainers etc. and it won't just be neglected. 1.2) unittests! How does anyone (including you) know that this actually works? 1.3) CI (continuous integrations), for example with GitHub actions. This makes sure the unittests are run every time, the package is deployed, it can be installed. Also CD (Continuous delivery) should be in place. 1.4) structure. It's currently a single file. The package should have a better logical structure, spreading out parts. 1.5) Python 2.7 support? This is dead. If there is no very strong reason to support it, consider dropping it. 1.6) consider using a pyproject.toml file to improve the packaging 1.7) make sure to drop any from ... import * imports! e.g. from math import *. It is highly discouraged to use it. 1.8) Don't use tabs for indentation! This is a no-go, spaces should be used throughout. Any decent IDE can automatically convert them.

1.9) consider sticking to PEP-8 and general not to "bend" the python syntax. There are autoformatters for the code. For example, a print is a function and does not have space before the (. So it's print(...), and not print (...). 1.10) there are a few things that miss the proper implementation and that Python offers improved ways. Python code that should be changed:

2) from the user perspective, there are a few things missing 2.1) the example has a from package import * at the beginning. This is discouraged and should absolutely not be used (the *) if not for special cases (e.g. config files). It can cause many bad issues. Also consider removing this from the __init__.py 2.2) there is no API documentation. How is a user supposed to know, which methods can be called, which arguments can be used? Consider setting up an auto api docs with sphinx and readthedocs 2.3) Is there something more about the project? e.g. people involved, long-term goals etc, the reason why this was built?

3) just an overall comment, maybe I miss out: why does it has to be capitalized strings that are added? It always seems a little antique to me. why not lowercase for example?

My overall impression of the package is that it looks more like a useful, private script that has been uploaded and slightly cleaned up. However, to provide a useful package (and "guarantee" support for the next few years -- be aware that as a user I don't want the package to work "just now" -- with the tools and work invested), there needs to be a whole lot more of things there, many which are lacking currently and can surely be improved on. This surely takes quite some works to accomplish but will reduce the workload in the future for maintenance. If this work is not provided, as a user, it will be highly assumed that the author will also not invest further work in case of possible bugs. Given that this package claims to be primarily a package to be used by people, not a novel algorithm, this points seem crucial to me.

My (personal) advice: writing a package is not sharing a private script. Be aware of the extra work required, that there is a whole science behind this (software engineering) and that it bears some 'responsibility', as people will rely on your work. Make sure you want to take this work! It's not always peaches and cream but roses and thorns.

I hope this comments can help you better orient and develop the package, they are written with the best intend.

aresio commented 4 years ago

Dear Jonas,

thank you for your precious comments. As you might have noticed, in the last weeks we applied several of the improvements that you suggested. As a matter of fact, Simpful is a relatively young project that gets constantly extended with novel functionalities. For instance, yesterday we introduced full support for Mamdani reasoning on arbitrarily shaped fuzzy sets (Simpful is one of the few libraries allowing that). We give priority to novel features than, say, refactoring.

Simpful was designed to be general-purpose, flexible, extensible and easy to embed into larger projects (e.g., pyFUME) where the novel algorithms are tested. Our aim is to create a simple, intuitive, and lightweight python libraries for fuzzy reasoning. We think that we achieved this goal. Because of its ever growing nature, it is not always the state-of-the-art from an architectural point of view: we promise we will try to keep the project as clean as documented as our energies will allow. :)

All the best,

Marco

jonas-eschle commented 4 years ago

Great to see this improvements, and I understand of course the status of the project. That also sounds like interesting news, indeed!