-
Notifications
You must be signed in to change notification settings - Fork 10
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
Make save function more universal to accept any number of 1D or 2D node or edge features #31
Conversation
…tures. Bugfix: Remove mutable default arguments
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.
This is great, thanks a lot for this contribution! 😄
I went over the code and left some small comments. I don't think there is any need for adding additional tests here as
- the saving functionality with default feature arguments is already tested in
weather-model-graphs/tests/test_save.py
Line 18 in 2456064
def test_save_to_pyg(): - we don't have any good functionality to add additional edge/node features that is now saveable with this more universal approach and could be used in a test (as far as I remember, let me know if I am wrong)
There are a couple small additional things that would be good to do before merging this:
- The linting seems to fail due to the code formatting. This should be easily fixed by just running the pre-commit hooks once.
- Go ahead and add an entry to the changelog! 🙂
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.
Tests and linting are all green now 🥳 Just want a small tweak to keep the formatting of the changelog consistent, then this is good to merge!
Co-authored-by: Joel Oskarsson <[email protected]>
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.
Everything good now, I'll merge this in. Thanks again for this contribution!
Describe your changes
This pull request contains three changes -- one major and two minor changes.
The major change modifies how edge and node features are converted from
pyg.data.Data
objects intotorch.Tensor
s before saving. If we assume that all node/edge features (features in the following) are numeric, a conversion to float32 will always work. Further, my proposed change assumes that the features can be either 1D (vector of edge lengths) or 2D (node position, vdiff vector between source and target nodes). Based on this, I propose a universal function that reshapes any 1D vector into a 2D column vector, which allows the concatenation of any number of features in a loop.Using the new function, we also make the saving of
len
as edge feature explicit by including it in the defaultedge_features
list.Two minor bugs are fixed:
hide_ticks
argument fornx.draw
seems to be deprecated because it caused an error with my networkx v 3.2Type of change
Checklist before requesting a review
pull
with--rebase
option if possible).Checklist for reviewers
Each PR comes with its own improvements and flaws. The reviewer should check the following:
Author checklist after completed review
reflecting type of change (add section where missing):
Checklist for assignee