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

Nit picks / Bugs #24

Closed
Teque5 opened this issue Jun 7, 2022 · 2 comments
Closed

Nit picks / Bugs #24

Teque5 opened this issue Jun 7, 2022 · 2 comments
Assignees

Comments

@Teque5
Copy link

Teque5 commented Jun 7, 2022

  1. Module should have a cplxmodule.__version__ defined.
  2. In your setup.py set the version=version and use this up top somewhere. This might look dirty but believe me it's the best way. We use this for gnuradio/sigmf and other projects:
import os

with open(os.path.join('cplxmodule', '__init__.py')) as derp:
    version = re.search(r'__version__\s*=\s*[\'"]([^\'"]*)[\'"]', derp.read()).group(1)
  1. I'm fairly certain that this line should be changed to allow passing a tuple of padding parameters like (5,7,) or whatever to padding:
self.stride[0], self.padding[0], self.dilation[0], # from this
self.stride, self.padding, self.dilation, # to this
  1. Due to the way that you are using relative imports (correctly) this project is compatible with Python 3.7+ and NOT prior versions. You can add an indicator for this in the setup. This was actually a bug related to your pip package reporting the same version as the version installed from git, but they were different and had different submodules. More reason to properly version.

I probably have a PR for you for a different feature, but I have to get it approved for release first.

ivannz added a commit that referenced this issue Jun 8, 2022
@ivannz
Copy link
Owner

ivannz commented Jun 8, 2022

@Teque5 Thank you for using the package and your issue!

1, 4. This was a gross oversight on my part. Thank you for pointing this out!
2. Thank you for the snippet! I decided to use a slightly different approach in setup.py: it creates a __version__.py during install.
3. The 1d convolution is supposed to take tuple singletons or ints, however I cannot recall any solid reason to hide exceptions related to bad argument values from the user.

I am going to ship these fixes soon bundled with minor updates to more modern pytorch version >=1.8.

update: Currently working on these and other updates in this branch, which has a very up-to-date name xD

update: See PR #25

@ivannz
Copy link
Owner

ivannz commented Jun 14, 2022

#25 has been merged and the update has been released on pypi.

@ivannz ivannz closed this as completed Jun 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants