FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

feat: add vesta viewer by stevenhua0320 · Pull Request #188 · diffpy/diffpy.structure · GitHub

  1. diffpy
  2. diffpy.structure

Repository navigation

feat: add vesta viewer - #188

Merged
sbillinge merged 5 commits into
diffpy:v3.4.0from
stevenhua0320:vesta_view
Mar 26, 2026
ConversationCommits (5)ChecksFiles changed
Merged

feat: add vesta viewer#188
sbillinge merged 5 commits into
diffpy:v3.4.0from
stevenhua0320:vesta_view

Conversation

stevenhua0320 commented Mar 24, 2026

Copy link
Copy Markdown
Contributor

@sbillinge ready to review. With the assistance of Claude code (with some training to group standard), it mimics the original atomeye app viewer and writes a vesta viewer. Moreover, since vesta supports its specific file format vesta, I also added a p_vesta for the parser with file that ends with vesta.

Additionally, I noticed that I missed one call of snakecase method (abcABG to cell_params), therefore, I also changed it in this PR.

stevenhua0320 changed the title Vesta view feat: add vesta viewer Mar 24, 2026

codecov Bot commented Mar 24, 2026 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.20%. Comparing base (4f7656c) to head (52748fb).
⚠️ Report is 9 commits behind head on v3.4.0.

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:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

sbillinge commented Mar 25, 2026

Copy link
Copy Markdown
Contributor

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.

stevenhua0320 commented Mar 25, 2026

Copy link
Copy Markdown
Contributor Author

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 diffpy.cmi and test whether the functionality of the viewer works properly.

sbillinge reviewed Mar 26, 2026
View reviewed changes

sbillinge left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Comment thread src/diffpy/structure/apps/vesta_viewer.py Outdated
#
# diffpy.structure by DANSE Diffraction group
# Simon J. L. Billinge
# (c) 2006 trustees of the Michigan State University.

sbillinge Mar 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Please check this block for accuracy

stevenhua0320 Mar 26, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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?

sbillinge Mar 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

we are moving to Copyright, "the contributors to diffpy.structure"

Comment thread src/diffpy/structure/apps/vesta_viewer.py Outdated
for backward compatibility.
"""

from __future__ import print_function

sbillinge Mar 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Do we need this line?

stevenhua0320 Mar 26, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Comment thread src/diffpy/structure/apps/vesta_viewer.py Outdated
lines."""
import os.path

myname = os.path.basename(sys.argv[0])

sbillinge Mar 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Let's take this opportunity to update code to latest standards with Path() and f-strings

stevenhua0320 Mar 26, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Comment thread src/diffpy/structure/parsers/p_vesta.py Outdated
# Constants ------------------------------------------------------------------

# Atomic Mass of elements
# This can be later when PeriodicTable package becomes available.

sbillinge Mar 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I think periodic table is available and used elsewhere. Could you maybe look into that?

stevenhua0320 Mar 26, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 reviewed Mar 26, 2026
View reviewed changes

sbillinge left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

nearly there. One last question.

Comment thread src/diffpy/structure/parsers/p_vesta.py Outdated

# End of class P_vesta

from diffpy.structure.parsers.P_xcfg import P_xcfg # noqa: E402, F401

sbillinge Mar 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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?

stevenhua0320 Mar 26, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 merged commit 68695b0 into diffpy:v3.4.0 Mar 26, 2026
6 checks passed
stevenhua0320 mentioned this pull request Mar 27, 2026
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

sbillinge sbillinge left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

Back | FazBrowse Home | New Git URL