📡 📉 Adding Tensorboard Tracker #416
Conversation
|
Pytorch's tensorboard has an add_scalars() function, wich allows to pass multiple scalars at once instead of having to do it on a for cycle with add_scalar(). Maybe it could be used here. Documentation: |
Thanks! This is totally an option, I just didn't use it here as I was unsure of a sensible main tag. Ideally it would be great to have train/eval separation perhaps using the main tag here. |
Using joinpath instead of / Co-authored-by: Charles Tapley Hoyt <cthoyt@gmail.com>
This commit does the following: 1. Imports TYPE_CHECKING from typing. This is always false during Python runtime, but set to true when a type checker like MyPy is being used to statically analyze the code 2. Uses a conditional to wrap importing the tensorboard stuff at the top. this means that during actual runtime, this is never imported 3. Make a type annotation for instance variables in the class. Specifically, this is for the SummaryWriter variable. Note that the thing that's stored in this variable is the class itself, so to refer to the class and not it being an instance of the class, we wrap the annotation with Type[]. Further, note that the annotation is done as a string. This makes it a lazy type annotation, which is good because it won't explode because it points to a package that will not be imported. Note: this is independent from my other comment, where I said this isn't really a good name for an instance variable.
better name for the summary writer Co-authored-by: Charles Tapley Hoyt <cthoyt@gmail.com>
|
@PyKEEN-bot test please |
|
I am having issues with changes the setup.cfg passing the precommit checks. I had wanted to add tensorboard to tensorboard =
tensorboardHowever this causes the check to fail complaining of trailing white space. |
|
I think this might be ok for an initial release of the tensorboard tracker. There is a bunch of extra stuff I think that could be done to improve and expand upon the experience however these would not affect core functionality and could be done at a later date. Stuff I would like to add includes embedding visualization and better separation between train/test metrics. |
|
@sbonner0 okay, then we're going to send the @PyKEEN-bot to test! It will give some feedback that you'll need to take care of (code style, doc style, etc.). If some of the feedback doesn't make sense, just tag me and I will explain what needs to be done |
|
@sbonner0 there's also the tradition of prefixing all pull requests' names in PyKEEN with two emojis. Please feel free to pick the ones you'd like :) |
Such responsibility! Is there some rule for choosing appropriate emojis? |
Nope, just do what feels right |
|
@sbonner0 please let me know about the tags (see #416 (comment)) and we can finish this up and merge it |
Hi @cthoyt - tags have now been removed - unused hangover from having used mlflow as the template for the TB tracker. |
|
@PyKEEN-bot test please! |
|
@PyKEEN-bot test again |
|
@sbonner0 thank you for your contribution! I know this took a long time but it's much appreciated |
|
@cthoyt Sorry it took me ages to finish off! Super happy to finally be a contributor though. I think there are some ways we could improve the TB tracker so I'll open appropriate issues and get working on a PR |
|
We're gearing up to make a release soon, so feel free to make new issues or a new PR as you battle test this! Thanks again :) |
This PR closes #383
Adding tensorboard as a results tracker.
What you need to test:
pip install tensorboardtensorboard --logdir=.data/pykeen/logs/tensorboard/Issues:
add_hparamsfunction, but I don't think it is quite what is needed here.add_embeddingfunction - would be great if the final embeddings (or a subset of them more realistically) could be added.Tasks: