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

Crowdsourced Hints - "0.2 release" #442

Merged
merged 28 commits into from
Aug 27, 2013
Merged

Crowdsourced Hints - "0.2 release" #442

merged 28 commits into from
Aug 27, 2013

Conversation

fephsun
Copy link
Contributor

@fephsun fephsun commented Jul 19, 2013

Applies a bunch of improvements to the crowdsourced hints xmodule.

  • Adds support for formula response types. To do this, the formula response has been refactored slightly, to expose some internal functionality.
  • Works with rounding tolerances specified in each response type.
  • Hint-collecting UI now displays all hints that the user saw on one view.
  • Closes numerous 500 errors.
    @rocha

@ghost ghost assigned dianakhuang Jul 19, 2013
@rocha
Copy link
Contributor

rocha commented Jul 22, 2013

Seems like a test is failing. Can you fix it?

@@ -876,6 +876,27 @@ def get_score(self, student_answers):

# TODO: add check_hint_condition(self, hxml_set, student_answers)

def answer_compare(self, a, b):
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not compare_answer? That way is more consistent with the naming of other methods.

@rocha rocha closed this Jul 22, 2013
@rocha rocha reopened this Jul 22, 2013
@rocha
Copy link
Contributor

rocha commented Jul 22, 2013

I closed the PR accidentally, reopening.

variables = samples.split('@')[0].split(',')
numsamples = int(samples.split('@')[1].split('#')[1])
sranges = zip(*map(lambda x: map(float, x.split(",")),
samples.split('@')[1].split('#')[0].split(':')))
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably worth holding on to a version of samples.split('@') since you seem to use it a number of times.

@rocha
Copy link
Contributor

rocha commented Aug 2, 2013

@fephsun do you think this branch is ready to be merged? if so, can you fix the tests?

@fephsun
Copy link
Contributor Author

fephsun commented Aug 2, 2013

Actually, can we roll the wizard-style UX into this, as well? I'll push that up in a bit. Just have to fix some pep/pylint errors.

@fephsun
Copy link
Contributor Author

fephsun commented Aug 2, 2013

Just added the new UX. The pep8 and pylint errors aren't in code that I touched, so I think this is good.

@fephsun
Copy link
Contributor Author

fephsun commented Aug 7, 2013

@rocha Merge now?

@rocha
Copy link
Contributor

rocha commented Aug 7, 2013

Let me take a look first. Thanks.

On Wed, Aug 7, 2013 at 9:58 AM, Felix Sun notifications@github.com wrote:

@rocha https://github.com/rocha Merge now?


Reply to this email directly or view it on GitHubhttps://github.com/edx/edx-platform/pull/442#issuecomment-22252850
.

@fephsun
Copy link
Contributor Author

fephsun commented Aug 16, 2013

@auraz @vaxXxa
This xmodule is moving under the Blades team soon, so you should take a look at it, and ask me questions about any confusing or bad code that I wrote.

@vasylnakvasiuk
Copy link
Contributor

@fephsun, ok. We will review this code at Monday? Is it ok?

@fephsun
Copy link
Contributor Author

fephsun commented Aug 16, 2013

@vaxXxa Yeah, Monday is fine. Thanks for getting back to me so quickly.

with this problem's tolerance.
"""
return compare_with_tolerance(
evaluator(dict(), dict(), a),
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use {} everywhere instead of dict():

In [6]: %timeit dict()
1000000 loops, best of 3: 277 ns per loop

In [7]: %timeit {}
10000000 loops, best of 3: 44.2 ns per loop

@fephsun
Copy link
Contributor Author

fephsun commented Aug 26, 2013

@valera-rozuvan Still waiting for the js/coffeescript review. I will be around until this Wednesday. Afterwards, I think @rocha can answer questions.

@valera-rozuvan
Copy link
Contributor

@fephsun I have looked over the CoffeeScript files. They look good. Can you please look into what is causing the build to be unstable? Otherwise, good to merge 👍

@fephsun
Copy link
Contributor Author

fephsun commented Aug 27, 2013

@valera-rozuvan It's pep8/pylint violations from other peoples' commits. I'll deal with this problem today, and merge.

fephsun added a commit that referenced this pull request Aug 27, 2013
Crowdsourced Hints - "0.2 release"
@fephsun fephsun merged commit b5e1d57 into master Aug 27, 2013
@fephsun fephsun deleted the felix/formula-hints branch August 27, 2013 17:26
@Lyla-Fischer
Copy link

Congrats! Good luck on the post-deploy support!

chrisrossi pushed a commit to jazkarta/edx-platform that referenced this pull request Mar 31, 2014
…style

Added fix for answer status and show answer to stay in the same place
antoviaque referenced this pull request in open-craft/edx-platform May 15, 2015
Fix typo in requirements file, making "pip install" work again
xavierchan pushed a commit to xavierchan/edx-platform-1 that referenced this pull request May 30, 2019
modify price text for program
Sujeet1379 pushed a commit to chandrudev/edx-platform that referenced this pull request Nov 17, 2022
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.

8 participants