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

Question about radius in BOWSR readme example #467

Open
janosh opened this issue Sep 21, 2022 · 7 comments
Open

Question about radius in BOWSR readme example #467

janosh opened this issue Sep 21, 2022 · 7 comments

Comments

@janosh
Copy link
Contributor

janosh commented Sep 21, 2022

This might be a dumb question. In the BOWSR readme example, why the rounding? And is the 0.6 factor an approximation for getting the larger element's radius?

expected_radius() might be better named expected_diameter() since it gives the sum of 2 radii.

## Relaxed Structure
To retrieve the relaxed structure:
```python
radius = max(round(expected_radius(compressed_optimizer.structure) * 0.6, 2), 1.1)
relaxed, _ = compressed_optimizer.get_optimized_structure_and_energy(radius=radius)
print(relaxed)
```

@shyuep
Copy link
Collaborator

shyuep commented Sep 21, 2022

@JiQi535 Pls fix and respond.

@janosh
Copy link
Contributor Author

janosh commented Sep 21, 2022

@JiQi535 In case you rename the function, I think it can also be simplified from this:

def expected_radius(struct):
    element_list = struct.composition.chemical_system.split("-")
    element_list = [get_el_sp(e) for e in element_list]
    ele1, ele2 = sorted(element_list, key=lambda x: x.atomic_radius)[:2]
    return ele1.atomic_radius + ele2.atomic_radius

to this:

def expected_diameter(structure: Structure) -> float:
    elem_1, elem_2, *_ = sorted(structure.composition.elements, key=lambda x: x.atomic_radius)
    return elem_1.atomic_radius + elem_2.atomic_radius

@janosh
Copy link
Contributor Author

janosh commented Sep 22, 2022

Btw, this function fails for unary structures.

ValueError: not enough values to unpack (expected at least 2, got 1)

@shyuep
Copy link
Collaborator

shyuep commented Dec 14, 2022

@JiQi535 I suggest you fix these problems and submit a PR. We shouldn't let this issue fester for so long. This is not a difficult fix.

@JiQi535
Copy link
Collaborator

JiQi535 commented Dec 15, 2022

why the rounding? And is the 0.6 factor an approximation for getting the larger element's radius?

After checking through the codes, I believe this 0.6 factor is just a customizable factor to define the minimum allowed atomic distance in an optimized structure. It provides a reasonable guess of the shortest bond length in a structure that is not unphysical. As in the example of README, we provide a cutoff value for the minimum allowed atomic distance as argument to the method compressed_optimizer.get_optimized_structure_and_energy(radius=radius) . The returned optimized structure will have shortest bond length larger than this cutoff "radius".

To make it more clear, I can change the name of the "radius" variable to "cutoff_distance".

@YunxingZuo @shyuep Please kindly correct me if my understanding is not correct. Thanks!

@shyuep
Copy link
Collaborator

shyuep commented Dec 15, 2022

@JiQi535 The bigger issue is the unary structure failure. That can be easily fixed?

@JiQi535
Copy link
Collaborator

JiQi535 commented Dec 16, 2022

@shyuep The BOWSER optimizer doesn't have this kind of limitation. The previous example function expected_radius to get an estimated shortest allowed atomic distance in the README does have this limitation, and it has already been modified in the PR to work with unary structures.

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

No branches or pull requests

3 participants