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

Added Lab1 report #12

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

Added Lab1 report #12

wants to merge 6 commits into from

Conversation

VIGGEEN
Copy link
Collaborator

@VIGGEEN VIGGEEN commented Jan 19, 2020

No description provided.

@VIGGEEN VIGGEEN requested a review from johanhoffman as a code owner January 19, 2020 22:37
Copy link
Collaborator

@ajlace ajlace left a comment

Choose a reason for hiding this comment

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

Code and report looks good. Try to expand the tests for future assignments.

Try to include a link to a Google Collab file (see template at the master branch) so that we also can run your code.

As Lab 2 is included in this PR, it will not get merged until Lab 2 is graded.

Copy link
Collaborator

@ajlace ajlace left a comment

Choose a reason for hiding this comment

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

Other than the comments in the review, for future reference: Your tests need to be more exhaustive. Small matrices are not enough as they do not necessarily show how accurate your implementations are. Instead create larger random matrices and compare your results to the ones gotten by numpy's implementations.

All changes mentioned in this review are needed to pass lab 2.

" if (A[i, j] != 0): return False\n",
" return True\n",
"\n",
"def qrf_test():\n",
Copy link
Collaborator

Choose a reason for hiding this comment

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

The norms to test against mentioned in the lab instructions are missing: ||Q^tq - I|| and ||QR-A||.

These need to be added to pass the lab. Also consider testing the accuracy of your implementation to the one of numpy's.

"colab": {}
},
"source": [
"def ds(A, b):\n",
Copy link
Collaborator

Choose a reason for hiding this comment

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

You are here using numpy directly, which is not allowed. This assignment (assignment 3) needs to utilize the algorithms mentioned in the theory, see lecture notes part 3 for details.

@ajlace
Copy link
Collaborator

ajlace commented Feb 12, 2020

Lab 3 looks great, but lab 2 still needs to be updated: see the changes I requested above.

Copy link
Collaborator

@ajlace ajlace left a comment

Choose a reason for hiding this comment

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

Lab 4: Include a plot where you compare the exact solution to your approximation.

Otherwise it looks good.

Lab 2 still needs to be updated.

Copy link
Collaborator

@ajlace ajlace left a comment

Choose a reason for hiding this comment

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

Lab 5 looks good.

Lab 2 and Lab 4 still need to be updated.

"cell_type": "markdown",
"source": [
"### Monte Carlo quadrature over a unit interval ###\n",
"We check that the error converges towards zero as the number of samples increases. By inspection it is apparent that the convergence rate resembles 1/sqrt(samples)."
Copy link
Collaborator

Choose a reason for hiding this comment

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

To make it clear that the convergence rate resembles 1/sqrt(n) you need to actually plot 1/sqrt(n) as well. Or a log-log plot. Comparison is a must for you to strengthen your claim.

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.

2 participants