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

Update Orth_proj #162

Merged
merged 7 commits into from Oct 22, 2018

Conversation

Projects
None yet
3 participants
@Nosferican
Collaborator

Nosferican commented Oct 6, 2018

Not much of a re-write

  • Fixed tests
  • similar
  • in
  • Using view

Nosferican added some commits Oct 6, 2018

Update rst_files/orth_proj.rst
Not much of a re-write
- Fixed tests
- similar
- Using `view`

@Nosferican Nosferican requested a review from arnavs Oct 6, 2018

@Nosferican Nosferican referenced this pull request Oct 6, 2018

Open

Re-writes Tracker #160

7 of 53 tasks complete
@jlperla

This comment has been minimized.

Collaborator

jlperla commented Oct 6, 2018

I am not sure I like the use of view. We can teach people to sprinkle @view macros as part of an optimization step, but using aview is a step away from the math.

Lets stick with simple matlab style slicing

@Nosferican

This comment has been minimized.

Collaborator

Nosferican commented Oct 6, 2018

I reverted to copying array slicing. For these short cases it should be fine, but for more computational intensive lectures using view would cut down the computational time considerably. It might be worth considering taking a few things from the optimization steps for those lectures.

@jlperla

This comment has been minimized.

Collaborator

jlperla commented Oct 6, 2018

I just added something into the style guide on the views stuff. Basically, use matlab-style copying whenever possible, and then we can tell people to use @views where necessary. A drop in efficiency is worth the code clarity.

I also clarified the guiding principle in the style guide: we should maintain the direct correspondence between math and code even if the performance is lower. If it is so much lower that it really matters (after profiling/benchmarking!) then we can make an exception and explain it to people

On that note, lets add in equation numbers for lines of code while we do the rewrites. My comments are pretty much always going to be the same: if it looks exactly like the math, I will like it, and if it looks like code when the math was otherwise possible,, I won't :-) We want students to do the same thing, and when teaching we need to make the connections.

On that, just a reminder (which I don't think applies here) to use out-of-place functions virtually everywhere, as in https://github.com/QuantEcon/lecture-source-jl/blob/master/style.md#general-control-structures-and-code-organization There may be exceptions, but since inplace functions are (1) error-prone for new users who forget the .= in assignment, (2) incompatible with static arrays and many others; (3) further away from the math, I would prefer to avoid them unless it is really necessar.

@jlperla

This comment has been minimized.

Collaborator

jlperla commented Oct 6, 2018

Oh, I will add in a clarification on equation numbers in the style doc soon, but there is no need to add in equation numbers and the correspondence in the code if the code is clearly following a math block above it. In that case, the correspondence is implied.

The issue is when there is a block of code which uses an equation in the middle of it, or there as a few lines of code below some math blocks that use different formulas

@@ -946,11 +940,11 @@ basis by QR decomposition and project once more.
Py3 = Q * Q' * y

This comment has been minimized.

@jlperla

jlperla Oct 7, 2018

Collaborator

Sorry, I know you didn't make the change but in this block it has Q = Matrix(Q) but then has algebra which would make the conversion unnecessary. We should keep them in the returned type as long as possible. See addition to styleguide.

This comment has been minimized.

@arnavs

arnavs Oct 10, 2018

Collaborator

Yeah, when I stuck those in (to avoid issues from Q' not being a Matrix) it was often overkill. Definitely wasn't going for style.

This comment has been minimized.

@Nosferican

Nosferican Oct 19, 2018

Collaborator
Q, R = qr(X)
a = Q * Q' * y
Q = Matrix(Q)
b = Q * Q' * y
a ≈ b
false
a
3-element Array{Float64,1}:
  1.0000000000000004
  2.999999999999999 
 -2.9999999999999996
b
3-element Array{Float64,1}:
 -0.5652173913043473
  3.2608695652173907
 -2.2173913043478253

Matrix is necessary here.

Nosferican added some commits Oct 19, 2018

@Nosferican

This comment has been minimized.

Collaborator

Nosferican commented Oct 20, 2018

Ready for review.

# Normalize
U[:,i] = u / norm(u)
output = projection / norm(projection)

This comment has been minimized.

@jlperla

jlperla Oct 22, 2018

Collaborator

Can we just go return projection / norm(projection)?

This comment has been minimized.

@Nosferican

Nosferican Oct 22, 2018

Collaborator

That is a style guide issue. I could suppress the output =.

This comment has been minimized.

@jlperla

jlperla Oct 22, 2018

Collaborator

OK, let me add to the style guide not to assign a variable name right before returning.

I don't have strong feelings on whether there should be a return in circumstances like this. Sometimes the return makes things clearer for big functions, and sometimes it is completely unnecessar.

This comment has been minimized.

@Nosferican

Nosferican Oct 22, 2018

Collaborator

I think it is a good practice. Stefan for example has pushed for long functions to return nothing by default in future releases.

This comment has been minimized.

@jlperla

jlperla Oct 22, 2018

Collaborator

Sounds good

Nosferican added some commits Oct 22, 2018

@arnavs arnavs merged commit 848f262 into master Oct 22, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment