create and initialize structuredf.py by tinatn29 · Pull Request #1 · diffpy/diffpy.clusterrender

tinatn29 · 2026-01-23T22:09:12Z

create a class StructureDF (inherited from pandas DataFrame) to represent a local structure or cluster of atoms. will later add _from_structure and _from_file methods

codecov · 2026-01-23T22:11:39Z

Welcome to Codecov 🎉

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

Thanks for integrating Codecov - We've got you covered ☂️

sbillinge · 2026-01-26T21:39:48Z

@sbillinge

tinatn29 · 2026-01-26T23:06:00Z

@sbillinge I've edited the DocString (moved it to under init and each parameter starts with "the")

sbillinge

nice. Please see suggestions

sbillinge · 2026-01-27T04:12:11Z

src/diffpy/clusterrender/structuredf.py

-    Parameters
+    Methods
+    -------
+    from_structure(structure, site_index=None)


A cleaner API may be to have load_structure() and make it something that simply tests what the input is (pymatgen object or file-path/string) and behaves accordingly

sounds good! Would you add load_structure() as a function in diffpy.clusterrender.io or as a method in this class?

and in that case, should we make __init__() take something like structure_input as argument, and we specify in the DocString that structure_input can be a pymatgen Structure, pathlib.Path, or str?

right.

Regarding function or method, I think it is really a parser, so perhaps we should call it parse_data and have it as method in the class. If the only way to do it is on instantiation, then we would presumably make it a private method.

src/diffpy/clusterrender/structuredf.py

sbillinge · 2026-01-27T04:16:55Z

src/diffpy/clusterrender/structuredf.py

+        site_index : int, optional
+            The index of atom in the structure to be treated as the
+            central atom.
+        filename : str, optional


let's accept a pathlib.Path or a string. That is our kind of new standard. Also, we only need to have structure entry if we resolve this in the method but allow it to be a file path or a pytmatgen structure object. In general, we may want to pass it a diffpy or objcryst structure object too, later, and this will be more easily extended if we do it this way.

ok we can have load_structure() take care of that

src/diffpy/clusterrender/structuredf.py

tinatn29 · 2026-01-29T21:27:09Z

@sbillinge I addressed your comments in this commit (will make a_parse_data method next PR). Is this one ready to merge?

sbillinge · 2026-01-30T15:01:02Z

news/tn-structuredf.rst

@@ -0,0 +1,23 @@
+**Added:**
+
+* create StructureDF class (DataFrame) to store a cluster of atoms


Name change

src/diffpy/clusterrender/structuredataframe.py

sbillinge

Nice. Nearly there. After reading what it does, I wonder whether w should call it "ClusterDataFrame" since it denotes center atom and references everything to that? It is like a container for a cluster around a center atom?

But otherwise it looks good

tinatn29 · 2026-01-30T19:51:14Z

@sbillinge I changed the name to ClusterDataFrame and edit the news accordingly.

sbillinge

This is good. Are we missing tests?

We just have a constructor but this is still a function that could be tested. Since all it does is create a DataFrame, have it create one and test that this is equal to what you expect.

tinatn29 · 2026-02-02T17:01:17Z

@sbillinge can we add the constructor test later? Currently, instantiating this class requires the method parse_structure that I haven't written yet.

Alternatively, we could change __init__ slightly such that if the input is already a DataFrame it will just create the class without calling parse_structure, then I can add the test?

sbillinge · 2026-02-03T01:26:35Z

@sbillinge can we add the constructor test later? Currently, instantiating this class requires the method parse_structure that I haven't written yet.

Alternatively, we could change __init__ slightly such that if the input is already a DataFrame it will just create the class without calling parse_structure, then I can add the test?

I think this comment was before we talked, but basically, we want the test to fail until it passes. Failing tests are good.

tinatn29 · 2026-02-09T22:16:16Z

@sbillinge I started setting up the test for the ClusterDataFrame constructor. I've only added a few test cases here (very basic -- input is either a dictionary or a pandas DataFrame), but wanted to make a PR to make sure the test format looks alright before I add more test cases.

sbillinge

Thanks for this. Please see inline comments though.

sbillinge · 2026-02-10T03:33:23Z

tests/test_clusterdataframe.py

+        input_structure, site_index, expected_output = input_test_data
+        cdf = ClusterDataFrame(input_structure, site_index=site_index)
+
+    # check if the output matches the expected DataFrame


we don't need comments like this as they are a bit trivial.

sbillinge · 2026-02-10T03:36:12Z

tests/test_clusterdataframe.py

+
+from diffpy.clusterrender.clusterdataframe import ClusterDataFrame
+
+"""


This probably is working but it is a slightly different structure than our most recent standard. I think that Yuchen is writing good standard tests, and so is Caden. The standards make the tests easier to read for the reviewers because the intent becomes very clear, and the test to test the intent is right below it and so things become easier to review. Could I maybe ask you to check with those guys, or check their code and try and use that style? Thanks so much.

Caden sent me an example from cmi. Will try to get to it today!

pre-commit Black

34101bc

tinatn29 added 3 commits January 23, 2026 17:15

add news

d5499f7

add DataFrame initialization option

afc1b9c

add property constructor

a491d0b

tinatn29 requested a review from sbillinge January 26, 2026 21:40

edit Docstring

ba49fb5

sbillinge reviewed Jan 27, 2026

View reviewed changes

edit docstring

9325a9f

sbillinge reviewed Jan 30, 2026

View reviewed changes

tinatn29 added 2 commits January 30, 2026 14:49

change name and edit docstring

78c25fb

change class name in news

975e28d

sbillinge reviewed Jan 31, 2026

View reviewed changes

start writing test for class constructor

d9c2ede

sbillinge reviewed Feb 10, 2026

View reviewed changes

		@@ -0,0 +1,23 @@
		Added:

		* create StructureDF class (DataFrame) to store a cluster of atoms


		from diffpy.clusterrender.clusterdataframe import ClusterDataFrame

		"""

Conversation

tinatn29 commented Jan 23, 2026

Uh oh!

codecov bot commented Jan 23, 2026

Welcome to Codecov 🎉

Uh oh!

sbillinge commented Jan 26, 2026

Uh oh!

tinatn29 commented Jan 26, 2026

Uh oh!

sbillinge left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tinatn29 commented Jan 29, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sbillinge left a comment

Choose a reason for hiding this comment

Uh oh!

tinatn29 commented Jan 30, 2026

Uh oh!

sbillinge left a comment

Choose a reason for hiding this comment

Uh oh!

tinatn29 commented Feb 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sbillinge commented Feb 3, 2026

Uh oh!

tinatn29 commented Feb 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sbillinge left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

tinatn29 commented Feb 2, 2026 •

edited

Loading

tinatn29 commented Feb 9, 2026 •

edited

Loading