Skip to content
This repository has been archived by the owner on Sep 21, 2021. It is now read-only.

Arma plots #126

Merged
merged 2 commits into from
Sep 27, 2018
Merged

Arma plots #126

merged 2 commits into from
Sep 27, 2018

Conversation

ajozefiak
Copy link
Member

After completing this one I've realized that the last segment of plotting could be somewhat simplified. Potentially this simplification could be performed later?

@Nosferican Nosferican mentioned this pull request Sep 26, 2018
20 tasks
@jlperla
Copy link
Member

jlperla commented Sep 26, 2018

I guess it depends on how much they would be simplified. If you think that it would make the code much more clear, then it may be worth doing sooner rather than later. My main worry would be forgetting about it.

Can you create a new issue (or add to the existing metaissue) with a checklist of the ones you want to review, and a note on the change you would intend to make?

@arnavs
Copy link
Member

arnavs commented Sep 26, 2018

@ajozefiak Just want to confirm that you're not proposing structural changes to code (for example, a lot of our plots are wrapped inside kind of opaque plotting functions like gen_plots, and maybe it would be clearer to take them out). Because I think that kind of stuff we can hold off on until we start refactoring in earnest.

But, like @jlperla said, I don't see any reason not to make any other quick fixes.

@ajozefiak
Copy link
Member Author

@arnavs The last snippet of code has functions like plot_simulation and these seem a bit excessive for this one lecture. I can hold off on any such structural changes, but this was one particular lecture that stood out to me.

@jlperla
Copy link
Member

jlperla commented Sep 27, 2018

For sure. I think that kind of excessive complexity will be fixed in the rewriting pass

@arnavs
Copy link
Member

arnavs commented Sep 27, 2018

Plots look great, merging now.

@arnavs arnavs merged commit f5594ec into master Sep 27, 2018
@arnavs arnavs deleted the arma_plots branch September 27, 2018 19:00
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants