-
Notifications
You must be signed in to change notification settings - Fork 23
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
build: Decouple and minimise dependencies for visualisation #131
Conversation
I realise that the example notebooks need to be refreshed after this. I would prefer to do it in a follow-up MR since the notebooks need a major refreshment anyway, especially that the LHE files "used" in them are not available. We could get new notebooks in a notebooks directory as for other or packages, and then add a Binder button as well ... WDYT? |
Codecov Report
@@ Coverage Diff @@
## master #131 +/- ##
==========================================
- Coverage 62.82% 60.28% -2.54%
==========================================
Files 2 2
Lines 234 209 -25
Branches 59 56 -3
==========================================
- Hits 147 126 -21
+ Misses 76 73 -3
+ Partials 11 10 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
e78101d
to
bef54b6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is good, and solves a lot of problems. 🚀 I think the only thing that would be good to note/fix(?) for the future would be
that the old method allowed for LaTeX fonts
while using graphviz.graphs.Digraph.render
doesn't seem to do that
Is there a way to get LaTeX fonts? This is somewhat steering back into the questions of Issue #53, which I haven't taken the time to review and refresh myself on, so apologies if all of this should be obvious.
It must be possible since both packages in the end write DOT language files. I used what I'm doing in DecayLanguage. I'm putting your request down as a to-do to look into this soon, after a few days off ... In the meantime, if this goes in we're good tp proceed with the other MRs dependent on this. |
for more information, see https://pre-commit.ci
2b31d26
to
cbb8817
Compare
Hello @matthewfeickert, I finally remembered to re-instate a test as you had before, see your comment #131 (comment) above. Coverage is to be worked on but that's not specific to this MR, rather a general issue that coverage is missing for certain classes, etc. That's clearly outsidethe scope of this MR. This is an important MR as it makes dependencies easier and unblocks some dependent MRs. As for your comment #131 (comment) I would gladly make this a follow-up issue as it is a quality of life improvement, and something I can look into in the future. Again, a different matter, second priority. In short, I really think this is ready to go in asap. If happy please approve and I will make the follow-up issue. Thanks a lot. |
Follow-up issue created - #135 for discussion #131 (comment). |
Hi @matthewfeickert, I will merge this really shortly since the follow-up task is created and blocking this further is counter-productive and not helping anyone, certainly not the users. Coverage is a serious matter to deal with across the files, not just the 2-3 lines mentioned by codecov above ;-). |
Okay. 👍 I still have some concerns about trading one viz library for another that now changes how the visualizations look, but I'm willing to kick that down the road a bit to sometime when I have more time to do the work here for these concerns to matter. |
It is best to decouple as much as possible, in design, and with the dependency on Graphviz we now have for visualisation, we no longer need networkx, tex2pix and dot2tex. The package gets much lighter with no loss of functionality since, as I show in MR #130, it is trivial at present to visualize a graph directly in a notebook or simply save the graph in, say, PNG or PDF.
IMO this deals with #53.
Resolves #53