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

Deleting unused imports and variables #23

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

Conversation

YCC-ProjBackups
Copy link
Contributor

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.

Copy link
Contributor

@rosecers rosecers left a 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?

Copy link
Collaborator

@curiosity54 curiosity54 left a 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?
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this really missing?

Copy link
Contributor Author

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!

Copy link
Contributor

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

*removing this line

Copy link
Collaborator

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

@YCC-ProjBackups
Copy link
Contributor Author

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?

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?
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
from ..utils import quaternion_to_rotation_matrix # missing?

Comment on lines +420 to +425
radial_hypers = {
"radial_gaussian_width": radial_gaussian_width,
}
self.radial_basis = RadialBasis(
radial_basis_name.lower(), max_angular, **radial_hypers
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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)

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.

3 participants