Skip to content

added java, matlab, and octave codestubs to clearsolutions.py #870

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

Merged
merged 3 commits into from
Aug 21, 2017

Conversation

Alexanderallenbrown
Copy link
Contributor

No description provided.

Copy link
Member

@jhamrick jhamrick left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! 🎉

I have just a few requests to match the code style of nbgrader and to keep the code stubs consistent across languages.

@@ -10,7 +10,7 @@
class ClearSolutions(NbGraderPreprocessor):

code_stub = Dict(
dict(python="# YOUR CODE HERE\nraise NotImplementedError()"),
dict(python="# YOUR CODE HERE\nraise NotImplementedError()",matlab="%YOUR CODE HERE",octave="%YOUR CODE HERE",java="//YOUR CODE HERE"),
Copy link
Member

Choose a reason for hiding this comment

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

Please make the following changes:

  1. Add spaces between comment marks and the first character (e.g. % YOUR CODE HERE)
  2. Add a second line to each stub to throw an error, as in the python code stub
  3. Please place each code stub on a different line, e.g.
dict(
    python="# YOUR CODE HERE\nraise NotImplementedError()",
    matlab="% YOUR CODE HERE\n% code that throws an error",
    ...
)

@jhamrick jhamrick added this to the 0.6.0 milestone Aug 21, 2017
@Alexanderallenbrown
Copy link
Contributor Author

@jhamrick I've made an attempt to make the requested changes, but I'm having a heck of a time figuring out an elegant way to throw an exception in Processing, since it generally hides that from users for simplicity. The code stub may get a little bit complicated. Any advice on the best path forward here?

@jhamrick
Copy link
Member

I am not familiar with processing, so I'm not really sure. If there isn't a good option for throwing an error I'd say let's just leave it as only the comment.

Generally looks great, but one nitpick: please remove the space after the newline, otherwise you'll end up with code that doesn't have consistent indentation like:

% YOUR CODE HERE
 error('No Answer Given!')

@Alexanderallenbrown
Copy link
Contributor Author

Done. Thank you!

@jhamrick
Copy link
Member

Thanks!

@jhamrick jhamrick merged commit f000cbf into jupyter:master Aug 21, 2017
@sigurdurb
Copy link
Contributor

@Alexanderallenbrown
Can I ask you how are you throwing errors with the Octave kernel so it works with nbgrader?? I have tried several solutions but when I click the nbgrader "Validate" button that runs the autograder test cells I always get "Success, your notebook passes all the tests" even though the test cells clearly output an error message when I run them manually. Did you get this to work?

@Alexanderallenbrown
Copy link
Contributor Author

@sigurdurb

I haven't tackled that yet... part of the reason is that I don't use any autograded questions in my course. I am always having formgrader take me through each submission manually anyway, so it hasn't been a priority for the notebooks to gracefully pass all tests. I'd be happy to take a look though.

@sigurdurb
Copy link
Contributor

sigurdurb commented Oct 17, 2018

Thanks @Alexanderallenbrown for a quick response.
@DeepHorizons I saw on your blog that you had been using the Octave kernel with nbgrader, so I ask the same question to you: When in the Octave kernel I and click the nbgrader "Validate" button that runs the autograder test cells I always get "Success, your notebook passes all the tests" even though the test cells clearly output an error message when I run them manually. Did you get this to work? I have tried both assert and error("msg")

@Alexanderallenbrown
Copy link
Contributor Author

@sigurdurb I could be way off base here, but I'm wondering if this has to do with the "output type" that is assigned to Octave error when the octave kernel executes. I'm really not sure, but if you look here it appears that the validation system is looking for something that is explicitly called out as an error in the cell output. I'm poking around in the octave kernel, but I guess I don't know enough about what exactly the validator is looking for yet. I'll keep digging around.

@sigurdurb
Copy link
Contributor

sigurdurb commented Oct 17, 2018

@Alexanderallenbrown Yes, that is one, but it first goes to this line, that gets called from here
The problem seems to be that the output_type of a cell where for example one line in the autograder test cell is giving an error to the notebook stdout is always 'stream' and not 'error' like nbgrader wants. Could this somehow be changed in the kernel for Octave?
For example one of the 'outputs' in the metadata is outputs:[{'output_type': 'stream', 'name': 'stdout', 'text': 'error: wrong answer\n'}, {other dicts of outputs}, ...]
A hack solution would be to re.match this error, it would need to be in _extract_error in validator.py and determine_grade in utils.py. The output is the same for both assert(FALSE) and error("msg"), i.e. both the stdout messages start with "error: ". But the output.traceback is something in _extract_error that would also have to be implemented in the octave kernel right?

@Alexanderallenbrown
Copy link
Contributor Author

@sigurdurb yeah i bet we will have to elevate this upstream to the octave kernel to do it right. Happy to raise an issue... the octave kernel doesnt seem to have much to it, so we may be able to figure it out and submit a pull request there if someone in that circle doesnt have a quick solution.

@DeepHorizons
Copy link

@sigurdurb It's been a while since I have touched Octave + nbgrader. I do remember getting it to work at some point but I also remember it being limited. I'll look over old code and see if I can replicate what I had.

@sigurdurb
Copy link
Contributor

Hey,
I posted my solution for validating the Octave kernel in nbgrader in this PR: #1038 and this issue: #1039
btw, did anybody check or open an issue if this could be resolved in the Octave kernel? @Alexanderallenbrown

@Alexanderallenbrown
Copy link
Contributor Author

@sigurdurb sorry, haven't had a moment to dig into that yet. Sounds like you have a workable solution though?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants