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

Refactoring and visualization #1

Open
wants to merge 21 commits into
base: main
Choose a base branch
from

Conversation

bilabbasi
Copy link

The main features added from this PR are as follows:

  • a separate Metric class to simplify the FaceMetric, TangentMetric, etc.
  • a pooling operation for meshes
  • adds scripts to easily run experiments
  • adds scripts for visualization of meshes
  • the train scripts have been significantly refactors for ease of reuse through out the repo (and not to be used as just a stand alone script)

@bilabbasi bilabbasi changed the title Refactoring and Refactoring and visualization May 1, 2022
@trachelr
Copy link

trachelr commented May 2, 2022

Hi Bilal, thanks a lot for your PR.
Let me test it and do a quick review.

Copy link

@trachelr trachelr left a comment

Choose a reason for hiding this comment

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

Very nice improvement of the code!! I was able to include pytorch_geometric in the environment.yaml file to install it with conda. I installed the CPU version in Ubuntu18.04 (through WSL) but I wasnt able to try GPU support. I've train a model for few epochs in the segmentation task with COSEG and HumanSeg datasets but I wasn't able to get the FAUST dataset for correspondance tasks. The scripts for metric visualisation is working on COSEG but I wasnt able to run the script for pools visualisation. Is it supposed to work with autoencoders only? There's a few pytests errors as well, please see logs in test.txt.

Thanks again for your contribution!

README.md Show resolved Hide resolved
@@ -7,11 +7,17 @@ dependencies:
- python==3.8
- numpy==1.19.1
- matplotlib==3.3.0
- pytorch==1.5.0
- pytorch
Copy link

Choose a reason for hiding this comment

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

I think pytorch_geometric is missing, please see pyg package here
https://pytorch-geometric.readthedocs.io/en/latest/notes/installation.html

Copy link
Author

Choose a reason for hiding this comment

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

The pytorch_geometric installation has separate instructions in the README, since it requires some additional stuff. There's a specific note in the README indicating that the environment.yaml doesn't contain installation for pytorch_geometric.

Choose a reason for hiding this comment

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

my bad, I didn't read that note. I installed pyg version 2.0.4 and py38_torch_1.11.0_cu115
Let it like that if you'd like to keep the same installation version.

models/mesh_pool.py Outdated Show resolved Hide resolved
scripts/visualize_metric.py Outdated Show resolved Hide resolved
scripts/visualize_pools.py Outdated Show resolved Hide resolved
tests/test_compute_face_area_and_angle.py Outdated Show resolved Hide resolved
train/train_args.py Outdated Show resolved Hide resolved
scripts/visualize_metric.py Outdated Show resolved Hide resolved
weights = torch.ones(len(v))
for j in range(len(v)):
metric = m[j].t().mm(m[j])
eigs = metric.eig()[0][:, 0]
Copy link

Choose a reason for hiding this comment

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

There's a warning here, please use torch.linalg.eig

Copy link
Author

Choose a reason for hiding this comment

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

That's strange; I'm not getting an error. Just to make sure, is your PyTorch up-to-date? I just ran it on my side, and nothing.

Copy link

@trachelr trachelr May 10, 2022

Choose a reason for hiding this comment

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

I have version 1.11

scripts/visualize_pools.py Outdated Show resolved Hide resolved
@bilabbasi
Copy link
Author

bilabbasi commented May 8, 2022

Hey Romain, thanks for the feedback. I've addressed a lot of the small things you've mentioned and submitted the changes. For the FAUST installation, can you tell me what your error is? If it persists, then I can just add details on how to download/set up the FAUST dataset.

I've removed the mesh pooling for now, since it's clear it requires a bit more testing before being implemented.

Thanks for your review so far :)

@trachelr
Copy link

Hello Bilal :)
Thanks for your changes and sorry for the mess-up with the installation... actually I'm running the code in a windows environment. For the FAUST dataset, it's a license issue, you know I'm not allowed to download the dataset... let's skip this part. Did you run the tests? I saw a few errors, don't know if your changes fixed it...
If you're confident with your changes, we can merge.

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