chore: update pre-commit by danielsirakov · Pull Request #199 · diffpy/diffpy.structure

danielsirakov · 2026-05-31T00:37:18Z

I re-did this as I forgot the news file

danielsirakov · 2026-05-31T01:45:51Z

stevenhua0320 · 2026-05-31T01:52:49Z

@danielsirakov Have you run the pre-commit hook before you commit this? Seems that we have failing pre-commit test here. To do so, first do git pull origin pre-commit_edits and rerun pre-commit run --all-files, then make another commit to push everything to this PR.

sbillinge · 2026-05-31T16:56:17Z

I don't love the changes that docformatter is introducing here either, by capitalizing the type.

This could be our fault. It is probably not a good idea to use the str: whatever the string is formalism. More normal would be describe what the thing is and then what the type is. Please can you guys look into what good standards are for this and let's see if it is too much work to bring this legacy code up to those standards.

stevenhua0320 · 2026-05-31T17:57:14Z

@sbillinge I have looked at the docstring changes that changed by docformatter and they could be categorized into two. One is the descriptive docstring that set after the variable and the other is the string attribute variable. I suggest for some of the docstring that we only have the descriptive functionality(the former case), we could change the descriptive style. Eg.

"""float: Conversion factor from B values to U values."""

to

"""The conversion factor from B values to U values, as a float

For some of the attribute definition string(the latter case), what I suggest is shadow the type of attribute with a pair of quote. So we ca do for example

title = ""
    """Str: The default value for `title`."""

to

title = ""
"""``str``: The default value for `title`."""

sbillinge

I gave some thought here. Please can we try and make these fixes when finals are over and move this to a p3.14 release?

sbillinge · 2026-06-12T20:24:26Z


    BtoU = 1.0 / (8 * numpy.pi**2)
-    """float: Conversion factor from B values to U values."""
+    """Float: Conversion factor from B values to U values."""


@danielsirakov @stevenhua0320 we don't want string literals floating around in the code, so whenever we find things like this we want to clean them up. The correct fix in general is to move them to the docstring at the top of the function and put them into a standard format. Here, this may just be a constant and this should be changed from a string to a comment and placed above line 154.

Sounds good, I cleaned up most of them, but I just now realized there are a few that I missed so I'll go back to address them as well

sbillinge · 2026-06-12T20:30:57Z

        "END",
    ]
-    """list: Ordered list of PDB record labels."""
+    """List: Ordered list of PDB record labels."""


another floating string literal. It is documenting the static data list. Gemini suggested that even though it is not declared as self. it is still considered as a class attribute and so should be documented in the class docstring, something like:

class P_pdb(StructureParser): """Simple parser for PDB format. The parser understands following PDB records: `TITLE, CRYST1, SCALE1, SCALE2, SCALE3, ATOM, SIGATM, ANISOU, SIGUIJ, TER, HETATM, END`. Attributes ---------- format : str Format name, default "pdb". orderOfRecords : list The ordered list of PDB record labels. validRecords : dict The set of valid PDB record labels. """

We can discuss this more, but in general I think taking this advice and using it also in other places in the PR is the right approach

Sounds good, I followed your format for this case and I will make sure to move other similar string literals I find to be documented in the class docstring

sbillinge · 2026-06-12T20:33:11Z

    # instance attributes that have immutable default values
    element = ""
-    """str: Default values of `element`."""
+    """Str: Default values of `element`."""


this code seems just to be setting defaults, so the default values just have to be moved up to the attribute description in the docstring above and deleted from here, something like that.

Here is the format I've been using for moving defaults to the attribute description, let me know if this is fine or if I should go back and change it:

title : str String description of the structure, default "".

Descriptions always start with "The". Otherwise, this is good.

Would this be fine for all:

title : str The string description of the structure, default "".

Would this be fine for all:

title : str The string description of the structure, default "".

Yes, this is right, you need to inspect the edits and whereever the description of the variable is not starting with "The", make a change for that. Thanks!

I went in and quickly changed all descriptions in the "Attributes" docstrings in diffpy.structure to be in that format. I could also look more thoroughly and update the format for all descriptions in the repository if you'd like

sbillinge · 2026-06-12T20:33:34Z

    # default values for instance attributes
    title = ""
-    """str: default values for `title`."""
+    """Str: default values for `title`."""


this is default setting again.

sbillinge · 2026-06-14T04:34:01Z

@danielsirakov gentle ping on this.

danielsirakov · 2026-06-14T05:07:49Z

@danielsirakov gentle ping on this.

Sounds good, I'll make the changes first thing tomorrow morning and ping you when ready tomorrow.

danielsirakov · 2026-06-15T23:16:14Z

I made the reformatting edits and I also added an additional fix where the doc-formatter was capitalizing "bool", "float", and "ndarray", which I fixed by adding a non-cap line for doc-formatter in pyproject.toml. I fixed the issue I mentioned on Slack (#344) by moving all of the string literals in structure.py from # linked atom attributes into the attributes section of the class Structure(list) docstring, and I added Rundong's fix in a few more spots.

@stevenhua0320, ready to review

stevenhua0320 · 2026-06-16T01:02:11Z

        Coordinate system for the fractional coordinates `xyz` and
        the tensor of atomic displacement parameters `U`.
-        Use the absolute Cartesian coordinates when ``None``.
+        Use the absolute Cartesian coordinates when ``None``, the default.


let's make it None as the default instead of connecting it by a comma to make it clearer for future developer to read.

Would this be fine: Use the absolute Cartesian coordinates when ``None`` as the default.

Yes, you are right.

Thanks, just pushed the edit

danielsirakov · 2026-06-16T01:35:23Z

I just pushed the edits to the descriptions in the "Attributes" docstrings where I added "The" and I also added the change to line 97 of atom.py

stevenhua0320 · 2026-06-16T01:40:35Z


    element = _link_atom_attribute(
        "element",
-        """Character array of `Atom` types. Assignment updates


Please confirm whether it is a safe edit, in my opinion this is a not correct fix because originally we should have the middle string as a docstring for the argument in the python function. So, I don't think we should delete this. The description for this private function is referenced below.

def _link_atom_attribute(attrname, doc, toarray=numpy.array): """Create property wrapper that maps the specified atom attribute. The returned property object provides convenient access to atom attributes from the owner `Structure` class. Parameters ---------- attrname : str The string name of the `Atom` class attribute to be mapped. doc : str The docstring for the property wrapper. toarray : callable, Optional Factory function that converts list of attributes to `numpy.ndarray`. Use `numpy.char.array` for string attributes. Return a property object. """

How could I check to confirm whether it's safe? Should I bring the strings back for the other ones as well or just for element?

How could I check to confirm whether it's safe? Should I bring the strings back for the other ones as well or just for element?

Even though from my base knowledge it is incorrect, You could run the functional testing/ pytest to confirm whether it is correct. To fix this, you need to paste the deleted string back and rerun the pre-commit and see if it could do the right fix to the formatting.

I ran pytest just to check, and it seemed that my edits to _link_atom_attribute didn't actually end up breaking anything. I found that there were 10 failures, but I ran pytest on main as well, and it seemed that they showed up there too, meaning that they were related to a different PR (#187), and the other 219 tests passed with my edits. I'm not sure why this is or if it's something that pytest missed, and if you want, I could still try to reinsert the deleted string and see if the pre-commit reformats it correctly.

I tried re-inserting the string, but it led to black and docformatter going back and forth again, and as far as I'm aware this doesn't have any other easy fix besides removing the string as it is issue #344 (PyCQA/docformatter#344)

You can find a usage of this private function and see whether it could still be running properly. But from my observation here since you deleted one argument here, you only give the function one argument while it usually expects two arguments.

I see, is there any other way that you recommend for me to reformat the deleted string to bypass the docformatter and black issues?

@danielsirakov you could first try on this kind of edit:

label = _link_atom_attribute( "label", ( "Character array of `Atom` names. Assignment updates " "the label attribute of all `Atoms`. " "Set the maximum length of the label string to 5 characters." ), toarray=lambda items: numpy.char.array(items, itemsize=5), )

which bypass the triple quotes using quotes to concatenate the string. Then use this format to rerun the pre-commit hook to see whether we could bypass the bug.

Good idea, I'll give it a try

It worked! I'll make the same change to element as well and then push

sbillinge

Please see comments

sbillinge · 2026-06-16T05:32:16Z

    @property
    def Uisoequiv(self):
-        """float : The isotropic displacement parameter or an equivalent value.
+        """float : The isotropic displacement parameter or an equivalent


Please put into standard form. Don't lead with the type

sbillinge · 2026-06-16T05:32:36Z

    @property
    def Bisoequiv(self):
-        """float : The Debye-Waller isotropic displacement or an equivalent value.
+        """float : The Debye-Waller isotropic displacement or an


sbillinge · 2026-06-16T05:34:21Z

-    """int: Width of boundary around corners of non-periodic cluster.
-    Retained from the original AtomEye/XCFG parser for API compatibility.
-    VESTA handles periodicity natively so this value has no effect on output.
+    """Int: Width of boundary around corners of non-periodic cluster.


Make standard

sbillinge · 2026-06-16T05:42:32Z


    element = _link_atom_attribute(
        "element",
-        """Character array of `Atom` types. Assignment updates


I agree with @stevenhua0320 that we need to check that the tests are still working. Tests passing is not a test of tests working. For example assert True will pass but not test anything.

danielsirakov · 2026-06-16T23:47:52Z

I made a few more fixes to string formatting and also reinstated the strings in the format that bypasses triple quotes.

danielsirakov and others added 3 commits May 30, 2026 16:29

chore: update pre-commit

396748e

chore: update pre-commit

1e0cca8

[pre-commit.ci] auto fixes from pre-commit hooks

5849ebf

sbillinge reviewed Jun 12, 2026

View reviewed changes

danielsirakov added 2 commits June 15, 2026 14:56

chore: restructure floating docstrings and resolve pre-commit conflicts

62d854b

chore: restructure floating docstrings and resolve pre-commit conflicts

5bc77ca

stevenhua0320 reviewed Jun 16, 2026

View reviewed changes

chore: Reformatted all descriptions in Attributes docstrings

bd8ab53

stevenhua0320 reviewed Jun 16, 2026

View reviewed changes

sbillinge reviewed Jun 16, 2026

View reviewed changes

chore: fix more docstring format and reinstate docstrings

85910ec

Conversation

danielsirakov commented May 31, 2026

Uh oh!

danielsirakov commented May 31, 2026

Uh oh!

stevenhua0320 commented May 31, 2026

Uh oh!

sbillinge commented May 31, 2026

Uh oh!

stevenhua0320 commented May 31, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbillinge Jun 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danielsirakov Jun 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

sbillinge commented Jun 14, 2026

Uh oh!

danielsirakov commented Jun 14, 2026

Uh oh!

danielsirakov commented Jun 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

danielsirakov commented Jun 16, 2026

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!

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!

Choose a reason for hiding this comment

Uh oh!

sbillinge left a comment

Choose a reason for hiding this comment

Uh oh!

sbillinge Jun 16, 2026 •

edited

Loading

danielsirakov Jun 16, 2026 •

edited

Loading

danielsirakov commented Jun 15, 2026 •

edited

Loading