Implemented issue #5856 #6172
Implemented issue #5856 #6172
Conversation
|
|
||
| bottom = kwargs.pop('bottom', None) | ||
| label = kwargs.pop('label', None) | ||
|
|
||
| markerline, = self.plot(x, y, markerfmt, label="_nolegend_") | ||
| if (orientation == "vertical"): |
There was a problem hiding this comment.
@QuLogic Hi I think it need both vertical and horizontal check there or else it will plot marker lines twice, unless you meant a else statement instead of elif like:
if (orientation == 'horizontal'):
...
else:
...
There was a problem hiding this comment.
Parentheses, not condition:
if orientation == 'vertical':|
This is a duplicate of #6168 Can you please coordinate to get a single PR? I think the general comments I left there also apply to this PR. |
|
opps, didn't realize there was another one until i PRed |
|
Are both of you in CSCD01? There are some things I like better about this version. For example using strings instead of bool leaves a clear(ish) upgrade path to having stem plots that start at the top and go down and start at the right and go left. attn @vincent-cuenca |
|
Yup, looks like we are. I can add in @ItsRLuo's version of the code |
|
attn @solvents (the internet tells me you are a TA this year). We tend to see a number of these concurrent PRs (at least we have in past years). My inclination is requests the students work together on a single PR, but want to make sure that is consistent with what you need from the class side of things. |
|
I don't see a problem with the teams collaborating after submitting their PRs. Usually there is a bonus for completing a pull request, so in this sort of case we'd give the bonus to both teams provided they both contributed fairly to a single PR, I suspect. For evaluation, we mostly look at the work they did before creating the PRs. If the issue is only having one PR in the first place, it gets a little tricky (part of their learning objective is to research issues to implement independently). I can check with the course instructor if its feasible for us to check for duplicate PRs before they are created. I'll put some kind of update here when I have more information. |
|
Course instructor here. The teams will coordinate on the PR, incorporating all your feedback. Thanks! |
|
I do not have a problem with the duplicate PRs coming in. In most cases although they address the same problem they fix it in different and complimentary ways. I was just checking that I was not causing trouble by asking them to work together once code has hit the internet. |
Added a parameter in the argument: orientation that takes "horizontal" or "vertical" like @anntzer suggested.