-
Notifications
You must be signed in to change notification settings - Fork 1
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
Deleting unused imports and variables #23
base: main
Are you sure you want to change the base?
Conversation
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.
Hi @YCC-ProjBackups -- love this. Thank you for correcting my bad spelling.
All looks good but with some questions -- why are we adding new imports to projection coefficients?
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.
just wanted to make sure all the imports are okay
@@ -1,11 +1,9 @@ | |||
import numpy as np | |||
|
|||
from ..representations.radial_basis import RadialBasis | |||
from ..utils import quaternion_to_rotation_matrix # missing? |
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.
Is this really missing?
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.
Yes, everything still runs fine.
For the quaternion_to_rotation_matrix
, I am fairly certain it is missing. The reason for not crashing the program is probably because we never use DensityProjectionClass
calculator -- only EllipsoidalDensityProjection
. In EllipsoidalDensityProjection
, we use scipy.spatial.transform.Rotation.from_quat(...)
to convert quaternion to rotation type.
Please let me know if I missed anything!
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'm fine with removing it if it's no longer needed. It might be a relic from the first version.
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.
*removing this line
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.
yep, we can remove this line in that case
Replying to @rosecers, I have not added any imports (only removed unused ones) -- Github probably shows it as addition due to changed ordering of the imports. Let me know if you still have questions! |
@@ -1,11 +1,9 @@ | |||
import numpy as np | |||
|
|||
from ..representations.radial_basis import RadialBasis | |||
from ..utils import quaternion_to_rotation_matrix # missing? |
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.
from ..utils import quaternion_to_rotation_matrix # missing? |
radial_hypers = { | ||
"radial_gaussian_width": radial_gaussian_width, | ||
} | ||
self.radial_basis = RadialBasis( | ||
radial_basis_name.lower(), max_angular, **radial_hypers | ||
) |
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.
radial_hypers = { | |
"radial_gaussian_width": radial_gaussian_width, | |
} | |
self.radial_basis = RadialBasis( | |
radial_basis_name.lower(), max_angular, **radial_hypers | |
) | |
radial_hypers = {} | |
radial_hypers["radial_basis"] = radial_basis_name.lower() # lower case | |
radial_hypers["radial_gaussian_width"] = radial_gaussian_width | |
radial_hypers["max_angular"] = max_angular | |
self.radial_basis = RadialBasis(**radial_hypers) |
I went through all files inside anisoap folder and deleted unused variables and imports.
Additionally, some typos in the comments are fixed.
I created a new branch from the most recent main and avoided making any breaking changes, so everything should work the same way still.