ivannz / cplxmodule

Complex-valued neural networks for pytorch and Variational Dropout for real and complex layers.
MIT License
138 stars 27 forks source link

Nit picks / Bugs #24

Closed Teque5 closed 2 years ago

Teque5 commented 2 years ago
  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)

3. I'm fairly certain that [this line](https://github.com/ivannz/cplxmodule/blob/ff051d96a7465ce7f155850a90a3664101f2b032/cplxmodule/nn/modules/conv.py#L108) should be changed to allow passing a tuple of padding parameters like (5,7,) or whatever to `padding`:
```python
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 commented 2 years ago

@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!

  1. Thank you for the snippet! I decided to use a slightly different approach in setup.py: it creates a __version__.py during install.
  2. 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 commented 2 years ago

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