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

Remove 'thresh' option in fitwarp2d. #208

Closed
wants to merge 7 commits into from
Closed

Remove 'thresh' option in fitwarp2d. #208

wants to merge 7 commits into from

Conversation

d-lamb
Copy link
Member

@d-lamb d-lamb commented Sep 29, 2017

Singular-value decomposition of the basis vectors can return
singular values that differ by 8--10 orders of magnitude. The
default value of thresh (1e-5) would discard several of these smaller
values. That's a problem because the inverses of the singular
values are actually used to produce the solution in _svd. Discarding
the largest singular values didn't seem to help either. Since it
wasn't entirely clear why some of the singular values were being
discarded, I commented out that line of code, and the associated
documentation, so now we keep all of the singular values.

I also added a detailed explanation of what fitwarp2d, _svd, and
_mkbasis actually do. And of course, some tests.

If somebody with a lot of linear algebra experience is convinced
of a a good reason to discard some of these singular values, we
can revisit this (hence why I haven't removed the code, just
commented it out for now).

Singular-value decomposition of the basis vectors can return
singular values that differ by 8--10 orders of magnitude. The
default value of thresh (1e-5) would discard several of these smaller
values.  That's a problem because the _inverses_ of the singular
values are actually used to produce the solution in _svd. Discarding
the largest singular values didn't seem to help either. Since it
wasn't entirely clear why some of the singular values were being
discarded, I commented out that line of code, and the associated
documentation, so now we keep all of the singular values.

I also added a detailed explanation of what fitwarp2d, _svd, and
_mkbasis actually do.  And of course, some tests.

If somebody with a lot of linear algebra experience is convinced
of a a good reason to discard some of these singular values, we
can revisit this (hence why I haven't removed the code, just
commented it out for now).
@coveralls
Copy link

Coverage Status

Coverage decreased (-10.5%) to 52.603% when pulling 0fc6c84 on fix_warp2d into ade05f6 on master.

sisyphus and others added 6 commits November 1, 2017 00:24
Previously the first element of a list provided to 'cat' was used
to set the type of the output piddle.  Now it uses the "highest"
type if passed a mixed-type list.

Also added some tests for this new behavior, and improved the
documentation for cat, append, and glue to better highlight the
differences between then.
There seems to be a problem with threads and Windows and freeing
memory.  Previously the warp2d test would crash on Strawberry Perl
with a "free to wrong pool" error.

I followed the advice from BrowserUK in this PerlMonks thread:
http://www.perlmonks.org/?node_id=742205 to create a pass-through
routine to free the interolation kernel's memory.  Thanks for asking
the similar question 9 years ago, Rob!

These changes don't cause any tests to fail on my Mac, we shall see
on the AppVeyor.
@wchristian wchristian closed this Feb 27, 2018
@wchristian wchristian deleted the fix_warp2d branch February 27, 2018 20:55
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.

None yet

4 participants