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

validate user-defined variables don't conflict, warn if override #53

Closed ChristopherChudzicki closed 6 years ago

ChristopherChudzicki commented 6 years ago

This addresses #51:

For both FormulaGrader and IntegralGrader, if any user-defined variable/function overrides a default, raise an error that can be suppressed with suppress_warnings=True.

coveralls commented 6 years ago

Coverage Status

Coverage remained the same at 100.0% when pulling dcc8045a843b93711b36e062860a3fa26f3bf303 on validate-no-collisions into 1864a8c98584cbf18c1457ca5555dc7704271932 on master.

jolyonb commented 6 years ago

This looks really good. You've done a good job here!

I'd like to update the documentation in a few more places.

I think we should also check to make sure that there are no collisions inside variables and numbered_vars. A quick and dirty check would be if len(set(variables)) > len(variables). Pulling out the actual duplicates takes a bit more work.

jolyonb commented 6 years ago

Line 58 of docs/formula_grader.md needs updating to mention the warning suppression.

ChristopherChudzicki commented 6 years ago

Somewhere in the root documentation for Graders, we should add a note about suppress_warnings I added a brief note about validation and suppress_warnings to the main README.

For variables and constants, we should mention that you can override the defaults (do we even mention the defaults?) Yes, you mentioned the defaults in the README :)

I reorganized the FormulaGrader docs a little bit to group sampling functions and constants a bit closer together and to put blacklist/whitelist/required_functions/forbidden_strings all together.

I think it's a bit clearer now that suppress_warnings=True is required for overriding either constants or functions.

Now, regarding:

In numbered_vars, we mention that a specific variable takes precedence over a numbered variable. We should mention the suppress_warnings flag there.

and

Line 58 of docs/formula_grader.md needs updating to mention the warning suppression.

Currently:

grader = FormulaGrader(
    answers='a_{0} + a_{1}*x + 1/2*a_{2}*x^2',
    variables=['x', 'a_{0}'],
    numbered_vars=['a'],
    sample_from={
        'x': [-5, 5],
        'a': [-10, 10]
    }
)

does not raise an error, but it probably should, I suppose.

jolyonb commented 6 years ago

Ah. I saw that you were checking to make sure that numbered_vars and variables didn't collide. I'm guessing that a variable a and a numbered variable a will trigger a collision warning, but a variable a_{0} and a numbered variable a won't. Honestly, I don't think it's worth worrying about collisions between variables and numbered variables, as the system has an unambiguous way to deal with that.

ChristopherChudzicki commented 6 years ago

Ah. I saw that you were checking to make sure that numberedvars and variables didn't collide. I'm guessing that a variable a and a numbered variable a will trigger a collision warning, but a variable a{0} and a numbered variable a won't.

Exactly—that much was easy to test for.

Honestly, I don't think it's worth worrying about collisions between variables and numbered variables, as the system has an unambiguous way to deal with that.

Yay :)

I'll just delete that doc line about q, c, etc and delete the unused function, then this is ready.

ChristopherChudzicki commented 6 years ago

@jolyonb squashed and ready!

Nice job on the constants with edX, they were annoying =) Are they still present for numericalresponse?

jolyonb commented 6 years ago

No, they're gone completely :)

jolyonb commented 6 years ago

One last little thing sorry...

ChristopherChudzicki commented 6 years ago

@jolyonb I guess I just forgot to push the last commit, which is now pushed. This should in fact be ready.