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

Default to lattice from trajectory for analysis #198

Closed
stefsmeets opened this issue Nov 21, 2023 · 6 comments · Fixed by #225
Closed

Default to lattice from trajectory for analysis #198

stefsmeets opened this issue Nov 21, 2023 · 6 comments · Fixed by #225
Assignees

Comments

@stefsmeets
Copy link
Contributor

SitesData should prefer the lattice from the trajectory instead of model structure. i.e. just load fractional coords from ideal structure.

We could also make it so that the lattices must be the same, so that the user needs to take care of this. Alternative, perhaps scale input ideal lattice to trajectory to enforce lattices are the same.

@stefsmeets stefsmeets self-assigned this Dec 12, 2023
@stefsmeets
Copy link
Contributor Author

Hi @AstyLavrinenko , I'm looking at this, but I don't see any instances where it takes the structure lattice over the trajectory lattice. Do you have an example where this goes wrong?

@AstyLavrinenko
Copy link
Contributor

Hi @stefsmeets,

The issue isn't specifically tied to SitesData. I went through sites.py and transitions.py, and it seems like the trajectory lattice is being employed to calculate states and distances. No issues there.

However, when it comes to the density plot, things get a bit tricky. The problem arises, for instance, when dealing with the structure:

if structure:
    if structure.lattice == vol.lattice:
        plot_points(structure.cart_coords, structure.labels, fig=fig)
    else:
        plot_structure(structure, fig=fig)

This leads to plotting different lattices for the trajectory and the crystallographic positions extracted from the .cif file. Ideally, I'd like to visualize my lithium density around positions on the same lattice, but I find myself manually adjusting the lattice for the .cif structure to match my trajectory.

A similar problem appears during shape analysis. There's a warning, warn_lattice_not_close(self.lattice, test_lattice), but when it comes to calculating distances and cart_coords, self.lattice is the one in play. This can affect the size of sites that I'm looking at.

I think, users shouldn't be forced into lattice matchmaking. Therefore, we couldn't make it so that the lattices must be the same.

@stefsmeets
Copy link
Contributor Author

stefsmeets commented Dec 13, 2023

Thanks for the clarification, would this help for plotting? Meaning you can specify your custom lattice to override the vol/structure lattices?

plots.density(
    vol = volume,
    structure = structure,
    force_lattice = volume.lattice,
)

@AstyLavrinenko
Copy link
Contributor

Yes, I think it's perfect. And if someone wants to check visually the mismatch, structure.lattice could be forced

@stefsmeets
Copy link
Contributor Author

I have two suggestions, either make the sites, lattice and space group directly configurable:

unique_sites = [sites[0] for sites in symmetrized_structure.equivalent_sites]

sa = ShapeAnalyzer(
    sites=sites, 
    lattice=trajectory.lattice,
    spacegroup=symmetrized_structure.spacegroup,
)
sa.analyze_trajectory(trajectory)

Or pass the lattice and drag it along:

sa = ShapeAnalyzer.from_structure(structure)
sa.analyze_trajectory(trajectory, force_lattice=trajectory.lattice)

I think the first has my preference, the second is going to be really ugly to program, and conceptually does not fit with the class structure.

@AstyLavrinenko
Copy link
Contributor

I think we can go with the first option. And using ShapeAnalyzer.from_structure(structure) without forcing the lattice could be optional.

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 a pull request may close this issue.

2 participants