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

Normalized multidimensional arrays #776

Merged
merged 1 commit into from
Jan 22, 2020

Conversation

thisrod
Copy link
Contributor

@thisrod thisrod commented Jan 16, 2020

The option Manifold=sphere() is supposed to handle multidimensional arrays. This adds a failing test as a reminder that it doesn't yet.

@antoine-levitt
Copy link
Contributor

Just fix it :

retract!(S::Sphere, x) = normalize!(x)
change to x ./= norm(x)

@thisrod thisrod force-pushed the multisphere branch 2 times, most recently from a3140a7 to de87d0f Compare January 16, 2020 04:51
The constraint optimize(..., manifold=Sphere()) used to only normalize
vectors.  Extending it to arrays of any dimension is straightforward.
@thisrod
Copy link
Contributor Author

thisrod commented Jan 17, 2020

Just fix it :

Done, I think.

Copy link
Contributor

@antoine-levitt antoine-levitt left a comment

Choose a reason for hiding this comment

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

Great, thanks! Good to merge from my end

@antoine-levitt
Copy link
Contributor

Well actually: JuliaLang/julia#34239 so this would have been fixed by itself.

@thisrod
Copy link
Contributor Author

thisrod commented Jan 19, 2020

Is it worth adding a Julia version test, and using normalise! if it supports multidimensional arrays?

@antoine-levitt
Copy link
Contributor

No I think the current version is OK. The tests seem to fail though?

@thisrod
Copy link
Contributor Author

thisrod commented Jan 21, 2020

Is there a way to run the tests again? I suspect that I confused Travis by force pushing my branch while it was running.

@antoine-levitt
Copy link
Contributor

I think close and reopen?

@pkofod pkofod closed this Jan 21, 2020
@pkofod pkofod reopened this Jan 21, 2020
@codecov
Copy link

codecov bot commented Jan 21, 2020

Codecov Report

Merging #776 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #776   +/-   ##
=======================================
  Coverage   76.34%   76.34%           
=======================================
  Files          42       42           
  Lines        2021     2021           
=======================================
  Hits         1543     1543           
  Misses        478      478
Impacted Files Coverage Δ
src/Manifolds.jl 84.21% <100%> (ø) ⬆️

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 92e3910...93a2112. Read the comment docs.

@pkofod
Copy link
Member

pkofod commented Jan 21, 2020

Seems to be working :)

@pkofod
Copy link
Member

pkofod commented Jan 22, 2020

Thanks!

@pkofod pkofod merged commit 88cf8a5 into JuliaNLSolvers:master Jan 22, 2020
@pkofod
Copy link
Member

pkofod commented Jan 29, 2020

note to self: it's a good idea to put a seed in such a test :)

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

3 participants