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

Reorganize variable name parsing #27

Closed ChristopherChudzicki closed 6 years ago

ChristopherChudzicki commented 6 years ago

This is a rewrite of the variable name parsing to address #25

One of two forms is acceptable:

1. front + subscripts + tail
2. front + lower_indices + upper_indices + tail

is allowed, where:

So for example: XY__ab__c_ is allowed (front + subscripts) and ST_{ij34}^{bc} is allowed (front+lower_indices+upper_indices) but T_ijk^{123} is not allowed (front+subscriots+upper_indiices).

Using the language above, edX original is front+subscripts

coveralls commented 6 years ago

Coverage Status

Coverage increased (+0.0002%) to 99.9% when pulling da7b08fc2bc60f913e6d87c821e35c0ff41e1b32 on fix-subscripts into 0326f14a43f8167addccd7eebdb794724de8b754 on master.

jolyonb commented 6 years ago

That looks reasonable to me. I have no idea how edX will display something like abcstuff{ijk}, but we can certainly allow it in the parsing. Thanks for getting to this - it's been sitting on my todo list for a while...

ChristopherChudzicki commented 6 years ago

Glad it seems reasonable. I think that using abc_stuff_{ijk} is probably not a good idea, but allowing it in the parser seems natural and seems to make the logic simpler: now tensor notation is a strictly "extra stuff" added to end of original edX variable name.

I'm currently happy with the implementation, but I'd like to write some more tests before merging.

jolyonb commented 6 years ago

Indeed. Please include tests like a2bc_d3e__4fg_{ijk}!

ChristopherChudzicki commented 6 years ago

@jolyonb

Indeed. Please include tests like a2bc_d3e_4fg{ijk}!

Maybe you realized that wouldn't work :)

I had trouble implementing normal subscripts and tensor lower_indices simultaneously for the same variable. Since I don't think they would display well anyway, I don't want to worry about it.

I've updated the PR description. The parsing is now, I believe, what you originally envisioned and is fairly clean. So, please review and merge if no issues.