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

Added CReTA #775

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Added CReTA #775

wants to merge 2 commits into from

Conversation

travisdriver
Copy link
Collaborator

No description provided.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we remove this tsv file from the PR?

Each row in the matrix contains (xi,yi,xj,yj) where xi,yi and xj,yj
are coordinates for images i,j respectively afer correcting for camera intrinsics.
R_avg: 3X3XN matrix of absolute rotations. [iRw in our notation]
maxImages: Maimum No. of images in the dataset (used for indexing)
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: typo on

Maximum number of images in the dataset (used for indexing).

from typing import Dict, List, Optional, Tuple

import numpy as np
from scipy.io import savemat, loadmat
Copy link
Collaborator

Choose a reason for hiding this comment

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

let's import only the module, as import scipy.io, instead of importing the function.

https://google.github.io/styleguide/pyguide.html#22-imports

savemat(f"ta_input_{matches_hash}.mat", {"RT": RT, "edges": edges, "R_avg": R_avg, "matches": matches})
if not os.path.isfile(f"ta_output_{matches_hash}.mat"):
raise FileExistsError("Could not find CReTA translation averaging solution.")

Copy link
Collaborator

Choose a reason for hiding this comment

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

could we add a comment for L98 about what needs to be run in between L97 and L100?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you remove this .asv file and all the other .asv files? (autosaved files)

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