Skip to content
This repository was archived by the owner on Nov 27, 2023. It is now read-only.

Conversation

@aprokop
Copy link
Contributor

@aprokop aprokop commented Mar 22, 2019

Co-authored-by: Damien L-G dalg24@gmail.com

Not sure where to put it, so it's in scripts for now. I would like to carry it with the repo so it could be used anytime without access to Jupyter.

There are some quality of life improvements compared to original @dalg24's version:

  • Command line arguments support
  • Automatic calculation of the number of files to parse
  • The ability to either display or save to a file

Right now, there is no test for it, but I could add it if considered useful. For example, I could add a test for visualization driver to dump data, and run the script just to make sure it runs.

Co-authored-by: Damien L-G <dalg24@gmail.com>
@ghost ghost assigned aprokop Mar 22, 2019
@ghost ghost added the need review label Mar 22, 2019
Copy link
Contributor

@dalg24 dalg24 left a comment

Choose a reason for hiding this comment

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

I would have placed it in packages/Search/example/viz/ and added a requirements.txt that lists docopt, matplotlib, and numpy. No strong opinion on this though. I think it's good we finally start putting these scripts under version control. I just never was sure where would be most appropriate.

Note that now that #535 made it into master, it is trivial to implement a new visitor that output the matrix directly in the format you like instead of parsing the ones in DOT format.

@codecov-io
Copy link

codecov-io commented Mar 22, 2019

Codecov Report

Merging #538 into master will increase coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #538      +/-   ##
==========================================
+ Coverage   96.11%   96.11%   +<.01%     
==========================================
  Files         130      130              
  Lines        9516     9517       +1     
==========================================
+ Hits         9146     9147       +1     
  Misses        370      370
Impacted Files Coverage Δ
...es/Search/src/details/DTK_DetailsTreeTraversal.hpp 97.22% <0%> (+0.02%) ⬆️

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 6bfdf64...c72fc05. Read the comment docs.

@dalg24 dalg24 mentioned this pull request Mar 22, 2019
Copy link
Contributor

@dalg24 dalg24 left a comment

Choose a reason for hiding this comment

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

I see it is still a draft but I'd like request that me or someone other than Andrey post in the conversation one image produced by the script before we accept theses changes.

@aprokop aprokop marked this pull request as ready for review March 22, 2019 13:52
@dalg24
Copy link
Contributor

dalg24 commented Mar 22, 2019

plot
For the record, in the dev/ci container I had to do apt install python3-pip and pip3 install -r requirements.txt

@aprokop
Copy link
Contributor Author

aprokop commented Mar 22, 2019

@dalg24 I assume that in the container one is able to run only saving to file, and not in display mode, correct?

@dalg24
Copy link
Contributor

dalg24 commented Mar 22, 2019

@dalg24 I assume that in the container one is able to run only saving to file, and not in display mode, correct?

I only saved the plot. In principle I think it is possible if you run your container with -e DISPLAY=$DISPLAY -v /tmp/.X11-unix:/tmp/.X11-unix or similar but I didn't try cause I connected remotely and do not really want to bother with X forwarding.

@aprokop aprokop merged commit 3d468ea into ORNL-CEES:master Mar 28, 2019
@aprokop aprokop deleted the arborx_plot_script branch March 28, 2019 21:41
@ghost ghost removed the need review label Mar 28, 2019
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.

3 participants