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

Open source NengoSPA #295

Merged
merged 3 commits into from
Nov 15, 2023
Merged

Open source NengoSPA #295

merged 3 commits into from
Nov 15, 2023

Conversation

tbekolay
Copy link
Member

@tbekolay tbekolay commented Nov 7, 2023

Under the GPLv2 license.

Copy link

github-actions bot commented Nov 7, 2023

Coverage report

The coverage rate went from 96% to 96% ⬆️

100% of new lines are covered.

Diff Coverage details (click to unfold)

nengo_spa/modules/transcode.py

100% of new lines are covered (92% of the complete file).

nengo_spa/vocabulary.py

100% of new lines are covered (97% of the complete file).

nengo_spa/version.py

100% of new lines are covered (100% of the complete file).

@jgosmann
Copy link
Collaborator

jgosmann commented Nov 8, 2023

Can you provide some more information about the supposed patent on fractional binding? I'm a bit surprised about this as I have used fractional binding (or real-valued convolution powers) as early as 2016. To me the pure mathematical fact that a real-valued exponent can be used in the Fourier space also seems to be hardly patentable. Thus, I would suspect that the patent is tied to some particular application of the method (maybe the stuff @bjkomer worked on)? But if that is the case, I'm unsure whether that would warrant the removal of the basic method itself.

From a technical point of view, the removal is also incomplete because the algebras themselves implement the (fractional) binding. This PR is only removing the convenience operator overloading.

@celiasmith
Copy link
Contributor

Hey @jgosmann long time no chat! Hope you're doing well, we should catch up -- though github comments isn't the place to do it :). To your questions, here's the patent: https://patents.google.com/patent/EP3712824A1

To provide some context, we realized exactly your point here in our offline discussions, and felt it was best to be unambiguous as to whether or not this software was intended to grant any access to our SSP-related patents. To be on the conservative side, we figured it was safest to remove any support in that direction. So since we can't prejudge what a court might say about the relation between offering support for fractional binding and providing access to our patents, we figured we'd take it out.

@tbekolay tbekolay force-pushed the open-source branch 3 times, most recently from 0b7e1c9 to bd92661 Compare November 15, 2023 17:39
We will be open sourcing NengoSPA under the GPLv2 license.
Fractional binding is patent protected so we are removing it
so that users do not use patented systems without realizing it.
@tbekolay tbekolay merged commit 9c6fb07 into main Nov 15, 2023
8 checks passed
@tbekolay tbekolay deleted the open-source branch November 15, 2023 20:13
@jgosmann
Copy link
Collaborator

As this has already been merged, I feel some more comments/questions are in order:

You're saying:

To be on the conservative side, we figured it was safest to remove any support in that direction.

Personally, I would have considered printing a warning when potentially patented features are used. But I cannot judge the legal implications and I'm not using NengoSPA actively anymore, so I'm fine with removing that stuff. However, if the goal really is to remove any support in that direction, this PR misses to remove the actual implementation in the algebras.

Furthermore, if code, that is potentially affected by patents, is removed, shouldn't VTB also be removed (patent link)? Or is this different because it may be unlikely that the patent is actually granted at this stage?

@jgosmann
Copy link
Collaborator

I thought of myself still as a maintainer of NengoSPA (not necessarily the only one), but maybe I was under the wrong impression or you don't care. Either way, given how this PR, the related changes, and release have been handled, I see no point continuing to be a maintainer (if I still was considered one).

To me, the communication here is lacking. While @celiasmith provided some (much appreciated) context after inquiry, my technical concerns have not been addressed in any way. (To be clear: I'm not against the removal fractional binding in itself. But it appears to me that you didn't remove what you were intending to remove. Or if, it is not clear to me why exactly the given part was removed and this impacts my ability to make informed future decisions as a maintainer.)

Besides, this PR was merged without a proper review and same for the release. While a review is no guarantee that all mistakes are caught, even shortly skimming it brought up a few points that I'm not happy with. For example, python_requires was changed to >=3.8 without changing the classifiers that still declare Python 3.6 and 3.7 "support". More importantly, not mentioning the removal of a feature (it's a breaking change!) is a major oversight to me.

Given the lack of communication and the rushed release, which is not up to the standards that I strive for in projects I am maintaining, I'm neither able nor willing to be associated to this as a maintainer anymore.

@jgosmann
Copy link
Collaborator

@tbekolay Please remove me as maintainer on PyPI. I no longer do this myself as my role was changed from "owner" to "maintainer" today. Note that you likely need to replace the PYPI_TOKEN in the GitHub Action secrets for the release pipeline to continue to work.

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

Successfully merging this pull request may close these issues.

3 participants