-
Notifications
You must be signed in to change notification settings - Fork 3
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
Refactor Pathway class #305
Conversation
vasp_full_path is calculated from vasp_path_vol, whereas the test is performed using vasp_full_path. There is a mismatch in the resolution between the two Volumes. This seems like a bug in the tests. I simplify the code to use a single volume instead.
Hi @SCiarella could you review this pr please? It may seem like a bit much, but I think it is easiest to start with the Pathway refactor, most of the other changes follow from there. And then Volume next. Changes to both classes enabled a bunch of other small changes and corrections that I included (e.g. plots). I'm happy to talk you through it offline. |
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.
Looks excellent! 🚀
My only comment is that I am not convinced that wrapped_sites
should be optional. I would make it mandatory, or at least default on True
.
Co-authored-by: SCiarella <58949181+SCiarella@users.noreply.github.com>
Good point on the wrapping! I'll go through that part again with this in mind. |
This PR refactors the Pathway class. I endend up tackling a bunch of cascading issues. It changes the following:
.dims
attribute toPathway
. This simplifies a bunch of site transformations that before depended on passing a volumePathway
total lengthfrac_sites
/wrap
to methods that return sites instead of in-place updatesPathway.path_over_structure
returns list ofPeriodicSite
which simplifies return statement and gives more options to userTodo