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

[ENH] Added NoisyWatsonMap as subclass of VisualFieldMap to simulate natural deviations in retinal coorinate to dva translation #493

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

narenberg
Copy link
Contributor

Description

Please include a summary of the change and which issue it closes below. Include relevant motivation and context.

Closes #274

Type of Change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)

Checklist

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

For detailed information on these and other aspects, see the contribution guidelines: https://pulse2percept.readthedocs.io/en/latest/developers/contributing.html

@narenberg narenberg requested a review from mbeyeler May 26, 2022 23:45
@codecov-commenter
Copy link

codecov-commenter commented May 28, 2022

Codecov Report

Merging #493 (8df7ee7) into master (7a9da85) will increase coverage by 0.09%.
The diff coverage is 93.24%.

❗ Current head 8df7ee7 differs from pull request most recent head c0f8cd0. Consider uploading reports for the commit c0f8cd0 to get more accurate results

@@            Coverage Diff             @@
##           master     #493      +/-   ##
==========================================
+ Coverage   93.83%   93.92%   +0.09%     
==========================================
  Files          90       98       +8     
  Lines        7782     9082    +1300     
==========================================
+ Hits         7302     8530    +1228     
- Misses        480      552      +72     
Flag Coverage Δ
unittests 93.92% <93.24%> (+0.09%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pulse2percept/datasets/nanduri2012.py 86.20% <ø> (ø)
pulse2percept/model_selection/optimizers.py 85.71% <0.00%> (+0.57%) ⬆️
pulse2percept/models/setup.py 0.00% <0.00%> (ø)
pulse2percept/stimuli/__init__.py 100.00% <ø> (ø)
pulse2percept/datasets/beyeler2019.py 22.97% <50.00%> (ø)
pulse2percept/utils/parallel.py 87.50% <61.53%> (-6.84%) ⬇️
pulse2percept/utils/geometry.py 81.86% <70.47%> (-4.72%) ⬇️
pulse2percept/models/base.py 87.32% <80.00%> (-0.04%) ⬇️
pulse2percept/utils/optimize.py 92.59% <80.00%> (ø)
pulse2percept/stimuli/images.py 82.07% <81.81%> (+1.86%) ⬆️
... and 59 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 57dc2bc...c0f8cd0. Read the comment docs.

Copy link
Member

@mbeyeler mbeyeler left a comment

Choose a reason for hiding this comment

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

Let's discuss tomorrow if there's a way to add a noise option to the existing VisualFieldMap class or if we should wrap the latter.

But, at the very least, this needs a test case.

@mbeyeler
Copy link
Member

mbeyeler commented Jun 9, 2022

Hi @narenberg, I would much prefer "noise" being an attribute of any VisualFieldMap (see my latest commit). Still needs a test and more docstrings.

Currently, this won't work with Watson2014DisplaceMap, because its dva2ret depends on Watson2014Map. These two classes should just be fused - but I can do that later. I'm mainly putting this here to remind my future self of it.

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

Successfully merging this pull request may close these issues.

[ENH] Add Gaussian noise to RetinalCoordTransform
3 participants