This repository was archived by the owner on Nov 26, 2021. It is now read-only.
[RFC] Some clarifications for plotting code#238
Open
bjackman wants to merge 9 commits intoARM-software:masterfrom
Open
[RFC] Some clarifications for plotting code#238bjackman wants to merge 9 commits intoARM-software:masterfrom
bjackman wants to merge 9 commits intoARM-software:masterfrom
Conversation
added 9 commits
January 17, 2017 15:50
PEP8 doesn't seem to be clear on how this should be indented. However I feel pretty sure that breaking lines inside a destructuring expression (i.e. after the comma here) would be confusing to the majority of programmers.
I don't really understand this code but AFAICT you can plot the same column from multiple traces, i.e. len(columns) == 1 and len(traces) != 1. Not sure what role templates has here.
These two params have equivalent roles for plotting traces and DataFrames respectively. Move them next to each other in the docstring to make it more likely that users will notice this symmetry and understand the API.
- Rename `val` to `count`. This variable is a cadinality i.e. the length of a list. To me, `val` suggests an element of a list. - Add to docstring to explain intention of the function
Collaborator
|
LGTM and thanks for this! |
sinkap
reviewed
Feb 19, 2017
| TODO is the following correct? | ||
| Permute( | ||
| Len[traces] != 1 | ||
| Len[columns] = 1 |
Collaborator
There was a problem hiding this comment.
Yes, this is correct and should work when the same column is present in all the events (templates). I agree templates was a really bad name for the variable :(
Contributor
Author
There was a problem hiding this comment.
Great, I'll remove the "TODO" and amend. I've figured out what templates is (I think it's explained in another function) so perhaps I can add a comment for that too :)
Contributor
Author
There was a problem hiding this comment.
Actually, shouldn't templates have length 1 here? I.e. you have multiple traces; if they're DataFrames that all have a certain column then columns should have length 1, and if they're FTraces that all have a certain event then templates should have length 1?
sinkap
force-pushed
the
master
branch
3 times, most recently
from
6f40d93 to
a84dd23
Compare
valschneider
pushed a commit
to valschneider/trappy
that referenced
this pull request
Sep 28, 2018
…-change Executor & LisaTest interface change
valschneider
pushed a commit
to valschneider/trappy
that referenced
this pull request
Sep 28, 2018
valschneider
pushed a commit
to valschneider/trappy
that referenced
this pull request
Sep 28, 2018
Since ruamel.yaml issue ARM-software#238 is partially resolved, this code does not break the deserialization anymore.
valschneider
pushed a commit
to valschneider/trappy
that referenced
this pull request
Sep 28, 2018
Avoid using __setstate__ for now on recursive structures
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I've been confused a few times while using the plotting code. This is a set of docstrings and cleanups that I made while trying to understand it. I've marked this RFC because:
So I guess consider this low-priority :)