Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v3.4.0 #188 +/- ##
=======================================
Coverage 99.20% 99.20%
=======================================
Files 15 15
Lines 2501 2501
=======================================
Hits 2481 2481
Misses 20 20 🚀 New features to boost your workflow:
|
|
Thanks @stevenhua0320. Did you review the code and clean it up before sending? You may want to take a look and save me some time. |
|
Yes, I took the review of the code and did a bit cleaning for the code structure. I think the logic is right. Since we don't have a command line control to test this one, we might need to integrate it into |
sbillinge
left a comment
There was a problem hiding this comment.
This is really great. Thanks. Please see comments. I see there is really a lot of changes needed is we are going to update all string formatting so maybe this is not s great use of time, but if there are any places where it makes sense to do f-strings and it is not taking to long then please go ahead
| # | ||
| # diffpy.structure by DANSE Diffraction group | ||
| # Simon J. L. Billinge | ||
| # (c) 2006 trustees of the Michigan State University. |
There was a problem hiding this comment.
Please check this block for accuracy
There was a problem hiding this comment.
So, the license holder should be now University of California, Santa Barbara? For the group I think the major contribution are credited to DANSE Diffraction group?
There was a problem hiding this comment.
we are moving to Copyright, "the contributors to diffpy.structure"
| for backward compatibility. | ||
| """ | ||
|
|
||
| from __future__ import print_function |
There was a problem hiding this comment.
No, I don't think we need this. I have this in the previous commit since in the anyeye.py we also have this import, so I took this for granted. But I checked through where we are using this import and I found we are not using it anywhere. So, it could be safely removed. I would make this in the next commit.
| lines.""" | ||
| import os.path | ||
|
|
||
| myname = os.path.basename(sys.argv[0]) |
There was a problem hiding this comment.
Let's take this opportunity to update code to latest standards with Path() and f-strings
There was a problem hiding this comment.
As per the CLAUDE.md that you shared, I asked the agent to follow those guideline and also ask to replace any nasty strings to f-string as much as possible. See the latest commit.
| # Constants ------------------------------------------------------------------ | ||
|
|
||
| # Atomic Mass of elements | ||
| # This can be later when PeriodicTable package becomes available. |
There was a problem hiding this comment.
I think periodic table is available and used elsewhere. Could you maybe look into that?
There was a problem hiding this comment.
This is now in the p_xcfg.py, so now we could directly import it from this file. Would make this in the next commit.
sbillinge
left a comment
There was a problem hiding this comment.
nearly there. One last question.
|
|
||
| # End of class P_vesta | ||
|
|
||
| from diffpy.structure.parsers.P_xcfg import P_xcfg # noqa: E402, F401 |
There was a problem hiding this comment.
can we move this to the top of the file? Or it has to be here for some reason. If it has to be here I am worried that there is something wrong with our design. What is P_xcfg used for?
There was a problem hiding this comment.
Yes, it could be moved to the top, the P_xcfg is used to parse the xcfg files that used by atomeye previously. I also checked through whether we have any usage of P_xcfg in the current code and I found nowhere is used in p_vesta. Hence we could actually remove this line. It might because CLAUDE thinks we are editing over the script over the original p_xcfg and therefore maintain this import, but here we are not needed. I would make a commit reflecting this shortly.
@sbillinge ready to review. With the assistance of Claude code (with some training to group standard), it mimics the original
atomeyeapp viewer and writes avestaviewer. Moreover, since vesta supports its specific file formatvesta, I also added ap_vestafor the parser with file that ends withvesta.Additionally, I noticed that I missed one call of snakecase method (
abcABGtocell_params), therefore, I also changed it in this PR.