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

feat: Combine plots #24

Merged
merged 6 commits into from
Jul 31, 2020
Merged

feat: Combine plots #24

merged 6 commits into from
Jul 31, 2020

Conversation

coolalexzb
Copy link
Contributor

@coolalexzb coolalexzb commented Jul 27, 2020

This PR aims to provide solution to issue#21.

User can use the command

python run.py -c mle -b [numpy,jax] -u https://www.hepdata.net/record/resource/1267798?view=true -m [750,100]

to run severalcomputations in one-time run and get the combination plots of running results and separation plots of running result like the following:
截屏2020-07-27 下午1 51 40
截屏2020-07-27 下午1 52 31
截屏2020-07-27 下午1 52 16
截屏2020-07-27 下午1 52 24

Copy link
Member

@matthewfeickert matthewfeickert left a comment

Choose a reason for hiding this comment

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

I still need to go through and run it to understand the functionality, but these are some quick comments.

scripts/plot.py Outdated Show resolved Hide resolved
scripts/run.py Outdated Show resolved Hide resolved
scripts/plot.py Outdated Show resolved Hide resolved
scripts/plot.py Outdated Show resolved Hide resolved
@lgtm-com
Copy link

lgtm-com bot commented Jul 31, 2020

This pull request introduces 1 alert when merging f6919de into 11fc554 - view on LGTM.com

new alerts:

  • 1 for Unused import

@coolalexzb
Copy link
Contributor Author

 if bk == "tensorflow":
     tf.get_logger().setLevel("ERROR")

This code can remove the warning information on std console. If we don't use it, I will get warning information like this on my PC
截屏2020-07-31 上午10 10 58

@matthewfeickert Do we need to reserve this code?

@matthewfeickert
Copy link
Member

@matthewfeickert Do we need to reserve this code?

No. This doesn't matter and is just normal for TensorFlow.

@codecov
Copy link

codecov bot commented Jul 31, 2020

Codecov Report

Merging #24 into master will decrease coverage by 3.60%.
The diff coverage is 61.06%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #24      +/-   ##
==========================================
- Coverage   65.52%   61.91%   -3.61%     
==========================================
  Files           9       10       +1     
  Lines         467      470       +3     
  Branches       57       66       +9     
==========================================
- Hits          306      291      -15     
- Misses        143      160      +17     
- Partials       18       19       +1     
Flag Coverage Δ
#unittests 61.91% <61.06%> (-3.61%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/pyhf_benchmark/plot.py 55.00% <49.12%> (-18.95%) ⬇️
src/pyhf_benchmark/run.py 63.56% <56.52%> (-4.58%) ⬇️
src/pyhf_benchmark/manager.py 91.30% <91.30%> (ø)
src/pyhf_benchmark/stats.py 64.66% <100.00%> (+0.81%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 11fc554...5925334. Read the comment docs.

@coolalexzb coolalexzb merged commit 91549c6 into master Jul 31, 2020
@coolalexzb coolalexzb deleted the combine branch July 31, 2020 15:44
Comment on lines +83 to +91
def subplot(y_label, column, output, directory, filename):

x_value = output["_runtime"]
if y_label == "Network Traffic (bytes)":
y_value1 = output.get(column[0], [0] * len(x_value))
y_value2 = output.get(column[1], [0] * len(x_value))
plt.plot(x_value, y_value1, ls="--", label="send")
plt.plot(x_value, y_value2, label="recv")
plt.legend(loc="upper left")
Copy link
Member

@matthewfeickert matthewfeickert Jul 31, 2020

Choose a reason for hiding this comment

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

All the plotting code should get switched over to using the matplotlib subplots API that I sent you earlier.

Copy link
Contributor Author

@coolalexzb coolalexzb Jul 31, 2020

Choose a reason for hiding this comment

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

@matthewfeickert So all the graph plots will be in one picture?

Copy link
Member

Choose a reason for hiding this comment

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

No. There are lots of examples on using matplotlib.pyplot.subplots. Start with the docs https://matplotlib.org/3.3.0/api/_as_gen/matplotlib.pyplot.subplots.html and then there are tons of results on google.

Copy link
Member

Choose a reason for hiding this comment

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

@coolalexzb Okay, following up here finally (sorry). What I mean here is that using the plot.plot API can be problematic as it is global and so you have to worry about dealing with a universal canvas. It is more robust to use the plt.subplots API. So you could simply create a function with a more descriptive name that takes similar arguments but then generates a figure and axis pair from

fig, axis = plt.subplots()
ax.set_xlabel("Time (seconds)")
ax.set_ylabel(y_label)
# ...
return fig, axis

and soforth. This is a more robust solution and then actually allows you to return the fig and axis objects

fig, axis = your_function(y_label, column, output)
# Do other things with fig and axis if you need
fig.savefig(directory / filename)

@matthewfeickert
Copy link
Member

@coolalexzb There were things in here that didn't get properly rebased and now need to get removed again. Let's start having me review PRs from now on.

This was referenced Jul 31, 2020
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.

2 participants