pymatgen
pymatgen copied to clipboard
Add option to skip SpacegroupAnalyzer in FEFFDictSet
Summary
Allows passing of a low_symmetry_system keyword argument to FEFFDictSet which skips the SpacegroupAnalyzer runs in constructing the header.
Comments
- Frankly I think the entire feature should be deprecated. There's no reason to do such a heavy calculation just for a header comment. If the user wants to perhaps they should just pass this directly to the comment themselves? If you want, I can make these changes.
- It's unrelated, but I noticed that default parameters are stored in
.yamlfiles. This is really confusing- why not just store these as default cards in the__init__s? I'm also happy to modify if desired.
A few suggestions:
- Pls change low_symmetry to just skip_symmetry.
- Pls add a unittest (in test_sets.py) to test the new argument functionality.
A note about the rest. A symmetry calculation is actually fast. By specifying symmetry, we reduce the cost of the FEFF calculation itself. It is true you can put settings in the init instead of yaml. The reason why this is done this way is to have the parameters in a glance in a file. One can also modify the parameters quickly. The rationale is probably more valid in very complicated input files like VASP. I guess we will keep it here for now.
- Pls change low_symmetry to just skip_symmetry.
- Pls add a unittest (in test_sets.py) to test the new argument functionality.
@shyuep sure will do.
A note about the rest. A symmetry calculation is actually fast. By specifying symmetry, we reduce the cost of the FEFF calculation itself.
How so? It appears that the symmetry information from SpacegroupAnalyzer only comes into play in constructing the header comments. Did I miss something?
It is true you can put settings in the init instead of yaml. The reason why this is done this way is to have the parameters in a glance in a file. One can also modify the parameters quickly. The rationale is probably more valid in very complicated input files like VASP. I guess we will keep it here for now.
The issue I take with this is that the yaml files are not exposed in the documentation. It's not available at a glance for the average user who might not that the parameter files are there.
Do you mind if I take a shot at improving this? I think it'll be much clearer as default arguments in the __init__ of the e..g XANESDictSet.
@shyuep just bumping the conversation. I'd like your feedback on what I mentioned in my above comment before I proceed.
@x94carbone please see also #2505 and issue #2624 . Neither is directly related to this but you may wish to provide feedback.
@x94carbone @rkingsbury I am happy to revisit but I just want to highlight that there are good reasons to use YAML formats, not least because it is far easier at a glance to know what settings are used than parsing python code. As a compromise, I suggest you use RST literalinclude e.g., https://dannguyen.github.io/danssphinx-template/content/001-examples/500-partials.html to insert the YAML file?
@shyuep I'm not sure I agree, but ultimately it's your decision.
Before I make any further changes though, can you provide some feedback on my blocking question?
A note about the rest. A symmetry calculation is actually fast. By specifying symmetry, we reduce the cost of the FEFF calculation itself.
How so? It appears that the symmetry information from SpacegroupAnalyzer only comes into play in constructing the header comments. Did I miss something?
Thanks!
@x94carbone I think it is fine. Pls go ahead and make the changes. Re yaml vs in code, I think this is really a matter of judgment depending on how familiar one is with Python code. Of course you and I are not going to have an issue reading python code. But for someone new, it is easier to read yaml. Even I find it helpful to quickly just pull up two YAML files if I want to say, figure out what the differences between a MPXANESSet and MPEXAFSSet are. I should note that the documentation actually does show the settings. See the pasted doc from pymatgen.org today. You see CONFIG