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

Bugfix #48

Merged
merged 19 commits into from
Nov 9, 2018
Merged

Bugfix #48

merged 19 commits into from
Nov 9, 2018

Conversation

ChristophWWagner
Copy link
Member

Some bug fixes and extensions. Resolves issues #37, #39, #40, #41, #42, #44

Previous pull request was withdrawn.

Previously, the Bluestein optimization were not regarded in the complexity
model, effectively causing some trouble. Also reduce code duplicates in
forward and backward by deriving the backward from the forward routine.
Previously, an error was thrown when a tap position was specified that
exceeded the size of the register. As the tap configuration directly
corresponds to the generating polynomial -- and this always has a x^k
factor with k being the order of the polynomial (size of the register) --
it is sensible to also accept the full notation of the polynomial.
The commit that is the answer to life, the universe and everything
 - Rename .regSize -> .size
 - Rename .regTaps -> .taps
 - Rename .resetState -> .start
 - Add .period property reporting the period of the sequence generated by
   the register configuration
 - Remove `length`option from initialization as the intended feature behind
   this option is currently broken and was previously not documented to the
   user. Instead a description on how it could be introduced later-on was
   added as a comment at the end of __init__()
 - Remove default value for the `start` option to __init__() to make the
   actual register configuration explicit
Testing the previous interface extension commits resulted in false-positives
for these (and deducted) classes as a pip-installed version shadowed the
version-under-test. This is now resolved.
 - Add __init__(minType=<type>) option, applicable to all classes. This
   option allows the user to widen the input datatype to mitigate range
   overflows.
 - Introduce internal ._prepareInputArray() method to handle all input array
   related stuff at one place for ._forward(), ._backward() and their
   corresponding cython-type methods. This method now also includes the
   dimension checks and cleans up code clarity of .forward() and .backward()
 - Introduce TRANSFORM struct being a container to keep track of input-,
   intermediate- and output array data types and shapes.
@SebastianSemper
Copy link
Member

So commit 124cf96 "cleans up code clarity"? Does this introduce some much needed chaos? :3

@ChristophWWagner
Copy link
Member Author

Yeah. It's true. Totally clear to me. So sad.

Copy link
Member

@SebastianSemper SebastianSemper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we now have all these beautiful options, we should first document them all in the initialization of the base class.

 - Improved consistency of additions to **options during __init__:
   You should never ADD an options to the **options dict to avoid
   interference with the userspace. Instead, copy the dict in options
   and modify the copy. This is now enforced.
 - Rename 'forceInputAlignment' to 'forceContiguousInput'
 - Rename argument in estimateRuntime
 - Rename 'useFortranStyle' to 'fortranStyle'
 - Rename 'minType' to 'minFusedType' internally as the external parameter
   is a dtype and the internal a fused type number (indexed)
 - Modified parameter passing in base class instantiation of transposed or
   conjugated classes to apply them by-dictionary, which gets extracted
   with Matrix._getProperties. This is more future-safe
 - Modified class-dependent setters of options `forceInputAlignment`,
   `bypassAutoArray`, `widenInputDatatype` and `fortranStyle` to use direct
   assignments instead of utilizing the parameter passing.
@ChristophWWagner ChristophWWagner mentioned this pull request Nov 8, 2018
Copy link
Member

@SebastianSemper SebastianSemper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

traceback (most recent call last): File "setup.py", line 376, in <module> ext_modules=lazyCythonize(extensions) File "/usr/lib/python3.7/site-packages/setuptools/__init__.py", line 140, in setup return distutils.core.setup(**attrs) File "/usr/lib/python3.7/distutils/core.py", line 148, in setup dist.run_commands() File "/usr/lib/python3.7/distutils/dist.py", line 966, in run_commands self.run_command(cmd) File "/usr/lib/python3.7/distutils/dist.py", line 984, in run_command cmd_obj.ensure_finalized() File "/usr/lib/python3.7/distutils/cmd.py", line 107, in ensure_finalized self.finalize_options() File "/usr/lib/python3.7/site-packages/setuptools/command/build_ext.py", line 133, in finalize_options _build_ext.finalize_options(self) File "/usr/lib/python3.7/site-packages/Cython/Distutils/old_build_ext.py", line 167, in finalize_options _build_ext.build_ext.finalize_options(self) File "/usr/lib/python3.7/distutils/command/build_ext.py", line 140, in finalize_options ('plat_name', 'plat_name'), File "/usr/lib/python3.7/distutils/cmd.py", line 287, in set_undefined_options src_cmd_obj.ensure_finalized() File "/usr/lib/python3.7/distutils/cmd.py", line 107, in ensure_finalized self.finalize_options() File "/usr/lib/python3.7/distutils/command/build.py", line 105, in finalize_options if self.distribution.ext_modules: File "setup.py", line 150, in __len__ return len(self.c_list()) File "setup.py", line 138, in c_list self._list = self.callback() File "setup.py", line 204, in extensions compiler_directives=cythonDirectives File "/usr/lib/python3.7/site-packages/Cython/Build/Dependencies.py", line 1086, in cythonize cythonize_one(*args) File "/usr/lib/python3.7/site-packages/Cython/Build/Dependencies.py", line 1192, in cythonize_one result = compile_single(pyx_file, options, full_module_name=full_module_name) File "/usr/lib/python3.7/site-packages/Cython/Compiler/Main.py", line 725, in compile_single return run_pipeline(source, options, full_module_name) File "/usr/lib/python3.7/site-packages/Cython/Compiler/Main.py", line 513, in run_pipeline err, enddata = Pipeline.run_pipeline(pipeline, source) File "/usr/lib/python3.7/site-packages/Cython/Compiler/Pipeline.py", line 355, in run_pipeline data = run(phase, data) File "/usr/lib/python3.7/site-packages/Cython/Compiler/Pipeline.py", line 335, in run return phase(data) File "/usr/lib/python3.7/site-packages/Cython/Compiler/Pipeline.py", line 52, in generate_pyx_code_stage module_node.process_implementation(options, result) File "/usr/lib/python3.7/site-packages/Cython/Compiler/ModuleNode.py", line 143, in process_implementation self.generate_c_code(env, options, result) File "/usr/lib/python3.7/site-packages/Cython/Compiler/ModuleNode.py", line 379, in generate_c_code self.body.generate_function_definitions(env, code) File "/usr/lib/python3.7/site-packages/Cython/Compiler/Nodes.py", line 442, in generate_function_definitions stat.generate_function_definitions(env, code) File "/usr/lib/python3.7/site-packages/Cython/Compiler/Nodes.py", line 442, in generate_function_definitions stat.generate_function_definitions(env, code) File "/usr/lib/python3.7/site-packages/Cython/Compiler/Nodes.py", line 4839, in generate_function_definitions self.body.generate_function_definitions(self.scope, code) File "/usr/lib/python3.7/site-packages/Cython/Compiler/Nodes.py", line 442, in generate_function_definitions stat.generate_function_definitions(env, code) File "/usr/lib/python3.7/site-packages/Cython/Compiler/Nodes.py", line 1942, in generate_function_definitions self.generate_argument_parsing_code(env, code) File "/usr/lib/python3.7/site-packages/Cython/Compiler/Nodes.py", line 2626, in generate_argument_parsing_code self.type.opt_arg_cname(declarator.name))) File "/usr/lib/python3.7/site-packages/Cython/Compiler/PyrexTypes.py", line 2957, in opt_arg_cname return self.op_arg_struct.base_type.scope.lookup(arg_name).cname AttributeError: 'NoneType' object has no attribute 'cname'

Unfortunately I get this error with cython 0.29 and CPython 3.7.1. after removing some obvious and minor typos in Matrix.pyx and MLCiruclant.pyx :-/

@ChristophWWagner
Copy link
Member Author

I shouldn't have cherry-picked the docu stuff from the int16 branch where it was pushed accidentially. Could have been easier just to accept it there, when this branch was still working ¯_(ツ)_/¯

@SebastianSemper
Copy link
Member

SebastianSemper commented Nov 9, 2018 via email

@ChristophWWagner
Copy link
Member Author

ChristophWWagner commented Nov 9, 2018

I do what I should hae done in the first place: Merge in int16 and do both pull requests in one. Currently having IT problems. Hopefully the VM starts after lunch or there is some shit that will hit the fan at hp support ;)

@SebastianSemper
Copy link
Member

SebastianSemper commented Nov 9, 2018 via email

@ChristophWWagner
Copy link
Member Author

Thanks, should be fine now. Happy approving! :)

@ChristophWWagner ChristophWWagner merged commit 4ce6697 into master Nov 9, 2018
@ChristophWWagner ChristophWWagner deleted the bugfix branch November 9, 2018 13:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants