Nexus: update RMG analyzer - #6194
Conversation
|
FYI, unless you need it, please don't push every commit since it triggers the GitHub actions CI and might (speculating, but it is logical) rate limit other runs in progress that we need for approved PRs. You can group them, push daily to show progress etc. |
|
What are the plans to document the analyzer? |
|
See reply to similar comment on #6196 |
rcclay
left a comment
rcclay
left a comment
There was a problem hiding this comment.
Just some very slight nitpicks. Lots of code, but most of its seems like unobjectionable file parsing and boiler plate stuff.
| stress = self.results.stress | ||
| if stress is None: | ||
| return None | ||
| return stress*1e8/self.pressure_units[units] |
There was a problem hiding this comment.
Why is there a 1e8, instead of being subsumed into the unit conversion?
|
|
||
| all_modes = frozenset({'scf','nscf','relax'}) | ||
| relaxation_modes = frozenset({'relax'}) | ||
| pressure_units = MappingProxyType({ |
There was a problem hiding this comment.
Is there any point for supporting eV/A^3, Ha/Bohr^3, Ry/Bohr^3? Not fun units for looking at an output file, but great for enthalpy calculations.
brockdyer03
left a comment
brockdyer03
left a comment
There was a problem hiding this comment.
This is just a partial review since I don't have time to comb through all of the slop that GPT produced.
Overall, this PR has a wide array of problems. Some of the most notable are unnecessary checks, unnecessary code flexibility, poor efficiency (the entire file is stored into memory and iterated over in its entirety several times), poor formatting.
Additionally due to its heavy use of obj for the attributes of RmgOutputData, is nearly impossible to document the class automatically, thus meaning that future developers will need to either pray that the code was well-documented manually and then need to constantly update the docs as they update the code, or sift through the source to figure out what everything is supposed to be.
There are formatting issues with nearly every docstring in the file too that should be addressed.
Another problem is that basically every read class silently ignores invalid parts of the file, which can lead to a lot of challenges if there needs to be any debugging of a failed analysis.
|
@brockdyer03 please don't be perjorative (i.e. "slop"). I spent many passes creating and vetting this code, and it is similar to what I would write. If you inspect the prior PwscfAnalyzer, the current code has fewer issues of the types you raise. |
|
Review comments addressed |
4 participants
This PR generalizes and hardens the RMG analyzer class. Capabilities are limited to scf, nscf, and relax runs.
Many real input/output files from RMG are included. These are there to support testing here and for the roll-forward.
GPT-5.6 Sol was used