-
Notifications
You must be signed in to change notification settings - Fork 14
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
Minor fixes -- actually not so minor... #144
Conversation
…ssic. Added (hidden) flag in docstrings to hide parameters entirely.
…uite unstable and sometimes would fail. The trick is really to have a much bigger crop for calibration, there is no escape from that frankly. Other tweaks in this commit as wellas improved docstrings.
…orking yet even on cb 1.0.5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks great overall, let me know when it is ready for final review @royerloic !
|
||
if description is not None: | ||
# Parameters that are marked with (hidden) in their docstrings are 'hidden: | ||
if '(hidden)' in description: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you like to hide the hidden
parameters from documentation page and our saved json too? or those only meant to be hidden on GUI @royerloic ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried running tests but it seems we are missing some files in aydin.util.fast_correlation
like correlation.py
and __init__.py
. Almost all test cases return
from aydin.util.fast_correlation.correlation import correlate
E ModuleNotFoundError: No module named 'aydin.util.fast_correlation.correlation'
Maybe those are accidentally excluded from the commit. @royerloic could you update the PR by adding these missing files?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some tests are failing but I will merge this PR now and open issues for the tests failing so we can follow-up later on. Thank you @royerloic again for the PR.
Lots of improvements needed for the latest use-case. Please review :-)