chore: deprecated addNewAtom by stevenhua0320 · Pull Request #157 · diffpy/diffpy.structure

stevenhua0320 · 2026-02-19T18:51:17Z

@cadenmyers13 ready to review first. Note that this is the scenario that I described in the slack chat in the first case. We don't have any test for this function, so there will be no deprecation warning when we run pytest. But we are not sure whether in some other packages that we use the CamelCase one, so I still add this deprecation here.

(diffpy.structure) ~/dbs/diffpy.structure git:[deprecate-addNewItem]
pytest
============================================================================================================== test session starts ==============================================================================================================
platform darwin -- Python 3.14.3, pytest-9.0.2, pluggy-1.6.0
rootdir: /Users/huarundong/dbs/diffpy.structure
configfile: pyproject.toml
collected 137 items                                                                                                                                                                                                                             

tests/test_atom.py ..                                                                                                                                                                                                                     [  1%]
tests/test_lattice.py ............                                                                                                                                                                                                        [ 10%]
tests/test_loadstructure.py ......                                                                                                                                                                                                        [ 14%]
tests/test_p_cif.py .....................                                                                                                                                                                                                 [ 29%]
tests/test_p_discus.py .....                                                                                                                                                                                                              [ 33%]
tests/test_p_pdffit.py .........                                                                                                                                                                                                          [ 40%]
tests/test_parsers.py .............                                                                                                                                                                                                       [ 49%]
tests/test_spacegroups.py .....                                                                                                                                                                                                           [ 53%]
tests/test_structure.py .......................................                                                                                                                                                                           [ 81%]
tests/test_supercell.py ...                                                                                                                                                                                                               [ 83%]
tests/test_symmetryutilities.py .....................                                                                                                                                                                                     [ 99%]
tests/test_version.py .                                                                                                                                                                                                                   [100%]

============================================================================================================== 137 passed in 0.61s ==============================================================================================================

cadenmyers13

@stevenhua0320 thanks, please see inline.

I responded to your slack message about the tests. Please check that out and do your best to get the DeprecationWarning to show up when you run pytest

cadenmyers13 · 2026-02-19T21:00:52Z

src/diffpy/structure/structure.py

+        """
+        kwargs["lattice"] = self.lattice
+        a = Atom(*args, **kwargs)
+        self.append(a, copy=False)


remove all the content of this function and have it take the new function name so we dont have duplicate code, ie,

def addNewAtom(self, *args, **kwargs): """This function has been deprecated and will be removed in version 4.0.0. Please use diffpy.structure.Structure.add_new_atom instead. """ self.add_new_atom(*args, **kwargs): return

I have done a global search, we only have test for this which is all commented in the test.

# def test_addNewAtom(self): # """check Structure.addNewAtom()""" # return # def test_getLastAtom(self): # """check Structure.getLastAtom()""" # return

cadenmyers13 · 2026-02-19T21:04:13Z

Also looks like there is typo in your PR title 😉

codecov · 2026-02-19T21:17:06Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.95%. Comparing base (1b565c8) to head (52c99e6).
⚠️ Report is 12 commits behind head on v3.4.0.

Additional details and impacted files

@@            Coverage Diff             @@
##           v3.4.0     #157      +/-   ##
==========================================
+ Coverage   98.93%   98.95%   +0.01%     
==========================================
  Files          13       13              
  Lines        1878     1905      +27     
==========================================
+ Hits         1858     1885      +27     
  Misses         20       20

Files with missing lines	Coverage Δ
tests/test_structure.py	`99.79% <100.00%> (+0.01%)`	⬆️

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

sbillinge

this looks good. If there is no test, please can we add one? But as @cadenmyers13 says, please check if it is tested any other way.

stevenhua0320 · 2026-02-19T21:34:35Z

this looks good. If there is no test, please can we add one? But as @cadenmyers13 says, please check if it is tested any other way.

Let me try to add one, I have done a global search and we don't have it. Also for getLastItem function that I ought to create in the next PR.

sbillinge · 2026-02-19T21:51:19Z

Thanks guys... The code continuously improves!

…ssage.

stevenhua0320 · 2026-02-19T21:53:41Z

@cadenmyers13 ready to review. I have added a test to test the behavior of addNewAtom, and now gives deprecation warning on local.

(diffpy.structure) ~/dbs/diffpy.structure git:[deprecate-addNewItem]
pytest
============================================================================================================== test session starts ==============================================================================================================
platform darwin -- Python 3.14.3, pytest-9.0.2, pluggy-1.6.0
rootdir: /Users/huarundong/dbs/diffpy.structure
configfile: pyproject.toml
collected 139 items                                                                                                                                                                                                                             

tests/test_atom.py ..                                                                                                                                                                                                                     [  1%]
tests/test_lattice.py ............                                                                                                                                                                                                        [ 10%]
tests/test_loadstructure.py ......                                                                                                                                                                                                        [ 14%]
tests/test_p_cif.py .....................                                                                                                                                                                                                 [ 29%]
tests/test_p_discus.py .....                                                                                                                                                                                                              [ 33%]
tests/test_p_pdffit.py .........                                                                                                                                                                                                          [ 39%]
tests/test_parsers.py .............                                                                                                                                                                                                       [ 48%]
tests/test_spacegroups.py .....                                                                                                                                                                                                           [ 52%]
tests/test_structure.py .........................................                                                                                                                                                                         [ 82%]
tests/test_supercell.py ...                                                                                                                                                                                                               [ 84%]
tests/test_symmetryutilities.py .....................                                                                                                                                                                                     [ 99%]
tests/test_version.py .                                                                                                                                                                                                                   [100%]

=============================================================================================================== warnings summary ================================================================================================================
tests/test_structure.py::TestStructure::test_addNewAtom
  /Users/huarundong/dbs/diffpy.structure/tests/test_structure.py:145: DeprecationWarning: 'diffpy.structure.Structure.addNewAtom' is deprecated and will be removed in version 4.0.0. Please use 'diffpy.structure.Structure.add_new_atom' instead.
    expected.addNewAtom(atype="C", xyz=[0.1, 0.2, 0.3])

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
======================================================================================================== 139 passed, 1 warning in 0.61s =========================================================================================================

cadenmyers13 · 2026-02-19T22:17:54Z

@stevenhua0320 Looks great! Thanks! Can you please add a minimal docstring to the test for the deprecated function? Something like Duplicate test for the deprecated addNewAtom method. remove this test in version 4.0.0. Just to help future maintainers know whats going on

sbillinge

Please see inline comments. Also. Please, as usual, add comments that capture the cases being tested and what we expect to happen

sbillinge · 2026-02-19T22:17:48Z

tests/test_structure.py

+        expected = Structure(lattice=s_lat)
+
+        length = len(expected)
+        expected.add_new_atom(atype="C", xyz=[0.1, 0.2, 0.3])


This test is done but the code is a bit lazily written. Please can we think more carefully to make it more readable? For example, isn't assert len(expected) == len(actual)? And "object", what object?

Since add_new_atom would return nothing in the original function, we must have a variable to check the length of the Structure object before and after the method. I also changed the object variable to a more precise name to atom_object. See the latest commit.

stevenhua0320 · 2026-02-19T22:40:27Z

@cadenmyers13 also just as a side note, do we really need a test for this function? I did a global search and we only use this function but we don't have anywhere used in test. However, I don't think we actually need the test for this function.

    def getLastAtom(self):
        """Return Reference to the last `Atom` in this structure."""
        last_atom = self[-1]
        return last_atom

sbillinge

nearly there, just please see comment.

sbillinge · 2026-02-20T00:53:15Z

tests/test_structure.py

    def test_add_new_atom(self):
+        """Check Structure.add_new_item()"""
+        # Case: We initialize a Structure object, after calling
+        # add_new_item method, we check whether length of Structure


This is describing the test, but remember the test tests behavior so we want to describe the behavior we want. This would be better as something like

Case 1: valid atom added to an empty structure. Expect the atom list length to go from 0 to 1. Expect the atom attributes are successfully loaded.

Then we will likely need

Case 2: valid atom added to exissting atom list. Expect ....

and

Case 3: duplicate atom added to existing atom list. Expect ....

and some behaviors for invalid atoms maybe, if we can think of tests that don't take too long to write.

I'm not quite sure about the physical perspective here. Do we allow duplicated atom into the atom list (for Case 3)? If not then we might expect an error and give the user some message about it.

@sbillinge ready for review. Note that from my bad physical perspective what I can think of is we should not allow a same atom at the same position in the space. So, I add the case 3 in the test, refining the original function here.

sbillinge · 2026-02-20T00:55:01Z

tests/test_structure.py

-        expected.add_new_atom(atype="C", xyz=[0.1, 0.2, 0.3])
+        length = len(structure)
+        structure.add_new_atom(atype="C", xyz=[0.1, 0.2, 0.3])
+        expected = len(structure)  # length of structure should add by 1


remove comment. this is handled in the comment at the top. Is this LLM code? OK, to get help from an LLM if it is, but I want you in charge, so you are responsible for what is happening and not the chatbot.

stevenhua0320 · 2026-02-20T02:44:01Z

This is the pytest after the modification.

(diffpy.structure) ~/dbs/diffpy.structure git:[deprecate-addNewItem]
pytest
============================================================================================================== test session starts ==============================================================================================================
platform darwin -- Python 3.14.3, pytest-9.0.2, pluggy-1.6.0
rootdir: /Users/huarundong/dbs/diffpy.structure
configfile: pyproject.toml
collected 139 items                                                                                                                                                                                                                             

tests/test_atom.py ..                                                                                                                                                                                                                     [  1%]
tests/test_lattice.py ............                                                                                                                                                                                                        [ 10%]
tests/test_loadstructure.py ......                                                                                                                                                                                                        [ 14%]
tests/test_p_cif.py .....................                                                                                                                                                                                                 [ 29%]
tests/test_p_discus.py .....                                                                                                                                                                                                              [ 33%]
tests/test_p_pdffit.py .........                                                                                                                                                                                                          [ 39%]
tests/test_parsers.py .............                                                                                                                                                                                                       [ 48%]
tests/test_spacegroups.py .....                                                                                                                                                                                                           [ 52%]
tests/test_structure.py .........................................                                                                                                                                                                         [ 82%]
tests/test_supercell.py ...                                                                                                                                                                                                               [ 84%]
tests/test_symmetryutilities.py .....................                                                                                                                                                                                     [ 99%]
tests/test_version.py .                                                                                                                                                                                                                   [100%]

=============================================================================================================== warnings summary ================================================================================================================
tests/test_structure.py::TestStructure::test_addNewAtom
  /Users/huarundong/dbs/diffpy.structure/tests/test_structure.py:168: DeprecationWarning: 'diffpy.structure.Structure.addNewAtom' is deprecated and will be removed in version 4.0.0. Please use 'diffpy.structure.Structure.add_new_atom' instead.
    structure.addNewAtom(atype="C", xyz=[0.1, 0.2, 0.3])

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
======================================================================================================== 139 passed, 1 warning in 2.90s =========================================================================================================

sbillinge

Please see comments

sbillinge · 2026-02-20T03:37:07Z

src/diffpy/structure/structure.py

        *args, **kwargs :
            See `Atom` class constructor.
+
+        Raises


I think this is undesirable behavior. Let's allow the atom to be added. We could raise a warning. But I think it is on the user to get this how they want it. So I would be inclined to simply allow it.

sbillinge · 2026-02-20T03:38:00Z

tests/test_structure.py

+        assert atom_object.element == "C"
+        assert numpy.allclose(atom_object.xyz, [0.1, 0.2, 0.3])
+
+        # Case 2: valid atom added to existing atom list.


There is lots of copy-pasted code here. I think using parametrize is better

stevenhua0320 · 2026-02-20T04:52:14Z

@sbillinge ready for another review, I changed to UserWarning instead of error in the function. Furthermore, I didn't realize at the beginning that originally the test functions are built with unittest, so when we put pytest.mark.parametrize in the TestStructure class it would not work. A compromise is to put it outside of class and then it works.

build: deprecate addNewItem

9ed7fc6

cadenmyers13 reviewed Feb 19, 2026

View reviewed changes

stevenhua0320 changed the title ~~build: deprecate addNewItem~~ build: deprecated addNewItem Feb 19, 2026

build: change body of deprecated function

6e99082

stevenhua0320 changed the title ~~build: deprecated addNewItem~~ build: deprecated addNewItom Feb 19, 2026

sbillinge reviewed Feb 19, 2026

View reviewed changes

stevenhua0320 changed the title ~~build: deprecated addNewItom~~ build: deprecated addNewAtom Feb 19, 2026

build: add test cases for addNewAtom to test it & show deprecation me…

95138f7

…ssage.

fix: refine test code to avoid if logic

3f285b6

sbillinge reviewed Feb 19, 2026

View reviewed changes

test: add docstring and comments to the test.

1fc8a34

sbillinge reviewed Feb 20, 2026

View reviewed changes

stevenhua0320 added 2 commits February 19, 2026 21:35

fix: add more test cases and refine behavior of add_new_atom

cb7aec0

fix: add docstring for hyperparamter for add_new_atom

114f3d6

fix: fix comment with syntax error

40e8773

sbillinge reviewed Feb 20, 2026

View reviewed changes

stevenhua0320 and others added 2 commits February 19, 2026 23:49

fix: change behavior to warning and use pytest parametrize to test it.

8f8253c

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

15ccba1

fix: fix comment for UserWarning.

52c99e6

sbillinge merged commit ca9116a into diffpy:v3.4.0 Feb 20, 2026
4 checks passed

stevenhua0320 changed the title ~~build: deprecated addNewAtom~~ chore: deprecated addNewAtom Feb 20, 2026

Comments

Conversation

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

Uh oh!

Uh oh!

cadenmyers13 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!

cadenmyers13 commented Feb 19, 2026

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

sbillinge left a comment

Choose a reason for hiding this comment

Uh oh!

stevenhua0320 commented Feb 19, 2026

Uh oh!

sbillinge commented Feb 19, 2026

Uh oh!

stevenhua0320 commented Feb 19, 2026

Uh oh!

cadenmyers13 commented Feb 19, 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!

stevenhua0320 commented Feb 19, 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!

stevenhua0320 Feb 20, 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!

stevenhua0320 commented Feb 20, 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!

stevenhua0320 commented Feb 20, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

stevenhua0320 commented Feb 19, 2026 •

edited

Loading

codecov bot commented Feb 19, 2026 •

edited

Loading

stevenhua0320 Feb 20, 2026 •

edited

Loading