mitodl / mitx-grading-library

The MITx Grading Library, a python grading library for edX
https://edge.edx.org/courses/course-v1:MITx+grading-library+examples/
BSD 3-Clause "New" or "Revised" License
14 stars 9 forks source link

Version 1.2 Checklist #78

Closed ChristopherChudzicki closed 6 years ago

ChristopherChudzicki commented 6 years ago

To keep me a bit organized

@jolyonb This stuff seems like a good goal to set before version 1.2. It seems like most of the work is documentation.

Might resolve some of the other issues along the way if they are architecturally convenient.

ChristopherChudzicki commented 6 years ago

@jolyonb I'm going to start with #65, then will move to #71, but let me know if you want something different.

jolyonb commented 6 years ago

I've added in refactoring InvalidInput. Otherwise, I think the list looks good!

jolyonb commented 6 years ago

Something else to think about is javascript renderings of any new functions that we've exposed.

jolyonb commented 6 years ago

Whee! Our issue list is shrinking rapidly!

ChristopherChudzicki commented 6 years ago

@jolyonb I edited your comment about Pauli matrices and moved it to a new bullet.

This would be very easy to add by a simple keyword argument.

It might be slightly nicer to allow user_constants to be a list of dicts so that athors could do user_constants=[PAULI, OTHER_STUFF, {...personal stuff}], but that makes validation (warning re overlap) somewhat tricker.

Also, if keyword, need to decide on names of pauli matrices: X vs s_x vs S_x vs sigma_X vs whatever.

jolyonb commented 6 years ago

Ok, fair enough. User constants will be pretty unusual though. Maybe its better to just define my own library and define them there?

jolyonb commented 6 years ago

Potential issue: we should make it clear when row vectors and column vectors are inequivalent. For a lot of purposes, we treat them as equivalent (eg, v*M*v). For grading purposes, they have different shapes though. What happens if we make a column vector and attempt to multiply v*M*v?

jolyonb commented 6 years ago

Also, does the dot function from numpy work if you give it two column vectors? two row vectors? one of each?

ChristopherChudzicki commented 6 years ago

Also, does the dot function from numpy work if you give it two column vectors? two row vectors? one of each?

Good question, found some bugs. See #89

ChristopherChudzicki commented 6 years ago

Potential issue: we should make it clear when row vectors and column vectors are inequivalent. For a lot of purposes, we treat them as equivalent (eg, vMv). For grading purposes, they have different shapes though. What happens if we make a column vector and attempt to multiply vMv?

I am pretty comfortable having the grader treat rows ([[1, 2, 3]]) and columns ([[1], [2], [3]]) as unequal. However, I think that both of these should be equal to the vector [1, 2, 3].

(Possibly there could be configuration keys for this if anyone found it convenient, but I think that's how the default should behave)

jolyonb commented 6 years ago

I'm really confused by what you just said. The grader differentiates between row and column vectors, but both are equal to a row vector?

ChristopherChudzicki commented 6 years ago

Actually: Let's just treat rows and columns as different things. For grading purposes, and for domain issues. (So, cross would only accept vectors, not rows or columns.)

If you're worried that the answer is [1, 2, 3] and a student might enter [[1, 2, 3]], then just set max_array_dim=1. (I am thinking 1 should actually be the default. I suspect that vector-entry will be much more common than matrix-entry.)

jolyonb commented 6 years ago

Ok, so a vector will be technically different to a row vector, which is the transpose of a column vector?

Is the matrix multiplication of a matrix on a vector the same as a matrix on a column vector? Similarly for row vectors multiplying from the left.

Agreed that 1 can be the default.

ChristopherChudzicki commented 6 years ago

Some examples:

from mitxgraders.helpers.mitmath import MathArray

a = MathArray([1, 2])               # shape: (2, )
a_row = MathArray([[1, 2]])         # shape: (1, 2)
a_col = MathArray([[1], [2]])       # shape: (2, 1)
I = MathArray([[1, 0], [0, 1]])     # shape: (2, 2)

I*a                                  # shape: (2, )
a*I                                  # shape: (2, )
I*a_col                              # shape (2, 1)
a_col*I                              # raises ShapeError
I*a_row                              # raises ShapeError   
a_row*I                              # shape: (1, 2)

(All of the above behavior is straight out of np.dot--it's the same branch of MathArray's mul method.)

Re

Ok, so a vector will be technically different to a row vector, which is the transpose of a column vector? That is what I am suggesting. I think that trying to coerce single-row matrices and single-column matrices into vectors is going to be (1) logically hard to do consistently and (2) hard to implement.

I think that the vast majority of potential confusion is mitigated by having max_array_dim=1 the default: students (and authors) won't be able to enter higher-dimensional arrays unless the author has specifically set max_array_dim=2.

jolyonb commented 6 years ago

Ok, I'm happy with this. We should make a note of this in the documentation, too.

jolyonb commented 6 years ago

For "Implement a method to easily extend default constants", the easiest will be to use dictionary comprehensions to combine dictionaries as {**x, **pauli, **whatever} on the user's side. Alas, that only became available in python 3.5. Given that edX is planning to upgrade to python 3, I propose that we wait on this one. I'm marking off the checkbox.

jolyonb commented 6 years ago

I propose we get 1.2 zipped up and ready for use before we do documentation, as the zip file/version number/etc doesn't depend on docs at all. What do you think? (Can you tell I want to use this immediately?)

ChristopherChudzicki commented 6 years ago

Sure. If we put MatrixGrader and the associated sampling classes in the changelog, can we tag it with beta or something, so that if we find (during your initial use, or while writing documentation) that we want changes, we can change it without feeling too bad.

By the way: It is still possible to use MathArray sampling classes from FormulaGrader, but if we document this "feature" at all, my plan is to say "DON'T DO IT." MatrixGrader has strictly better support for matrices, e.g., configuration keys for negative exponents and shape errors.

ChristopherChudzicki commented 6 years ago

A few other thoughts:

jolyonb commented 6 years ago

Probably no point to a beta version - I can just zip things up as they currently stand.

I'm not worried about exposing the shape of quantities, or revealing that they're matrices. I think most situations will tell students exactly what shape something has anyway so they know what they're dealing with.

I like the idea of being able to choose what response students get to a wrong shape entry. This has potentially two flags (marked as wrong or raise error, and what message to give), but I think we can boil it down to the following options:

jolyonb commented 6 years ago

Resolved by #106.