Skip to content

API: move 'interp' out of spaces#1520

Merged
kohr-h merged 23 commits into
odlgroup:masterfrom
kohr-h:interp
Jan 21, 2020
Merged

API: move 'interp' out of spaces#1520
kohr-h merged 23 commits into
odlgroup:masterfrom
kohr-h:interp

Conversation

@kohr-h

@kohr-h kohr-h commented Aug 13, 2019

Copy link
Copy Markdown
Member

In the hope of getting it merged faster, I created this PR that takes the minimal action to remove 'interp' from the spaces (where I no longer think it makes sense) to corresponding operators.

Tests will follow.

Please review this version.

@pep8speaks

pep8speaks commented Aug 13, 2019

Copy link
Copy Markdown

Checking updated PR...

No PEP8 issues.

Comment last updated at 2020-01-21 22:12:09 UTC

@kohr-h

kohr-h commented Aug 14, 2019

Copy link
Copy Markdown
Member Author

Some additional context: Apparently the PR #1459 is too big to review, so I'm trying to break it down into smaller ones. This one is only concerned with moving the interp property from spaces to operators and functions that make use of it (really only a few).
This implies that all calls to <space or element>.sampling(...) and <space or element>.interpolation(...) have to be replaced by according function calls. These functions are pulled out of the FunctionSpaceMapping type operators, which were removed.

I haven't adapted the tests yet since that would mean moving a bunch of code around, blowing up the size of the changes substantially. That'll come as a second step.

@aringh aringh left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Review of the first 7 files, out of 11. So far I think it looks ok, but I don't think I have not understood all details. Detailed comments below.

I will try to review the rest shortly.

Comment thread odl/deform/linearized.py
Comment thread odl/deform/linearized.py
Comment thread odl/deform/linearized.py Outdated
Comment thread odl/discr/discr_ops.py
Comment thread odl/discr/discr_utils.py Outdated
Comment thread odl/discr/discr_utils.py Outdated
Comment thread odl/discr/discr_utils.py Outdated
Comment thread odl/discr/discr_utils.py Outdated
Comment thread odl/discr/discr_utils.py Outdated
Comment thread odl/discr/discr_utils.py

@aringh aringh left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I have no strong opinion on whether interpolation should be an operator or not. This looks overall fine to me.

Ps. The remaining part was shorter than I though. So I reviewed it right away 😄 Ds.

Comment thread odl/tomo/backends/skimage_radon.py
Comment thread odl/tomo/backends/skimage_radon.py
Comment thread odl/tomo/backends/skimage_radon.py
@kohr-h

kohr-h commented Oct 30, 2019

Copy link
Copy Markdown
Member Author

Thanks for the partial review, @aringh. I'll try to get the changes done asap.

@kohr-h

kohr-h commented Nov 12, 2019

Copy link
Copy Markdown
Member Author

Doctests now work again and make more sense, sorry for the noise. I'll do the unit tests next.

@kohr-h kohr-h force-pushed the interp branch 2 times, most recently from 16908af to e3b0fdf Compare January 17, 2020 21:50
Comment thread odl/test/space/tensors_test.py
Comment thread odl/test/discr/discr_utils_test.py
Comment thread odl/test/util/normalize_test.py
@kohr-h

kohr-h commented Jan 18, 2020

Copy link
Copy Markdown
Member Author

It's been a while. Tests are fixed now, with only minor adaptions. Ready for final review.

Changes since last review:

  • Nearest neighbor variants removed. Never used, and resulted in flaky tests.
  • Fix added for a PyTest issue when one of the parameters is a NumPy array. PyTest caches test parameters and uses == to check if it has seen a value already. That doesn't work for arrays.
    Caching is only used for scope='module' or higher, so the default scope still works.

@kohr-h kohr-h requested a review from aringh January 19, 2020 11:01

@aringh aringh left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Partial review. Review of:

  • examples/deform/linearized_fixed_template.py
  • odl/contrib/datasets/ct/mayo.py
  • odl/deform/linearized.py
  • odl/discr/init.py
  • odl/discr/discr_ops.py
  • odl/discr/discr_utils.py

Will try to continue with the rest later this week.

Comment thread odl/deform/linearized.py
Comment thread odl/deform/linearized.py
Comment thread odl/discr/discr_utils.py
Comment thread odl/discr/discr_utils.py
Comment thread odl/discr/discr_utils.py
Comment thread odl/discr/discr_utils.py Outdated
Comment thread odl/discr/lp_discr.py
Comment thread odl/discr/lp_discr.py
Comment thread odl/deform/linearized.py Outdated

@aringh aringh left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Over all looks good to me!

Comment thread odl/discr/discretization.py Outdated
Comment thread odl/discr/lp_discr.py
Comment thread odl/test/deform/linearized_deform_test.py
Comment thread odl/test/discr/discr_utils_test.py Outdated
Comment thread odl/test/discr/discr_utils_test.py
Comment thread odl/test/discr/discr_utils_test.py
Comment thread odl/test/discr/discr_utils_test.py
Comment thread odl/test/discr/discr_utils_test.py
@kohr-h

kohr-h commented Jan 21, 2020

Copy link
Copy Markdown
Member Author

Thanks for the review, @aringh! Remaining stuff is fixed, will merge after CI.

@kohr-h kohr-h merged commit c332803 into odlgroup:master Jan 21, 2020
@kohr-h kohr-h deleted the interp branch January 21, 2020 22:30
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.

3 participants