Deprecate `Diffraction_object`, rename to `DiffractionObject` #165

bobleesj · 2024-11-09T17:27:17Z

No description provided.

bobleesj · 2024-11-09T17:28:04Z

src/diffpy/utils/scattering_objects/diffraction_objects.py

Two deprecation messages written 1) docstring level 2) class level

bobleesj · 2024-11-09T17:28:36Z

src/diffpy/utils/scattering_objects/diffraction_objects.py

@@ -18,6 +19,20 @@


 class Diffraction_object:
+    """FIXME: Add class docstring.
+
+    .. deprecated:: 3.5.1


Following the numpy deprecation standards:

Ref: https://numpydoc.readthedocs.io/en/latest/format.html#deprecation-warning

codecov · 2024-11-09T17:28:54Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.27%. Comparing base (2559946) to head (7c73140).
Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #165      +/-   ##
==========================================
+ Coverage   99.17%   99.27%   +0.09%     
==========================================
  Files           7        8       +1     
  Lines         243      274      +31     
==========================================
+ Hits          241      272      +31     
  Misses          2        2

Files with missing lines	Coverage Δ
tests/test_diffraction_object.py	`100.00% <100.00%> (ø)`

bobleesj · 2024-11-09T17:29:22Z

src/diffpy/utils/scattering_objects/diffraction_objects.py

+        `DiffractionObject` to follow the class naming convention.
+    """
+
+    warnings.warn(


Warning produced at the run-time level:

This looks good. Here I would maybe begin with the words "Diffraction_object is deprecated and will be removed....."

bobleesj

Ready for review - learning how to handle deprecation more gracefully in our group.

sbillinge · 2024-11-10T10:33:54Z

We also need the new class and it's tests. I guess it makes sense to have it on the same PR?

erge branch 'deprecate' of https://github.com/bobleesj/diffpy.utils into deprecate

bobleesj · 2024-11-10T15:47:05Z

doc/source/api/diffpy.utils.scattering_objects.rst

@@ -26,3 +26,12 @@ diffpy.utils.scattering_objects.diffraction_objects module
    :members:
    :undoc-members:
    :show-inheritance:
+
+
+diffpy.utils.scattering_objects.diffraction_object module


API module created for DiffractionObject

bobleesj · 2024-11-10T15:48:41Z

tests/test_diffraction_object.py

@@ -0,0 +1,265 @@
+from pathlib import Path


Same comment above - new file since file name changed

bobleesj · 2024-11-10T15:51:23Z

src/diffpy/utils/scattering_objects/diffraction_object.py

+
+
+class DiffractionObject:
+    """


I included docstrings for this class. Initially, I also created docstrings for each method within this class docstring above __init__. However, when I rendered the documentation locally, I noticed duplicate entries, as the docstring for each method is provided already. Therefore, here, I’ve only included descriptions for the Parameters and Aattributes

In this PR, none was modified except the class name.

I see. So this is a direct copy-paste to duplicate it and then change the name, so we have both and they are identical to each other?

Yes, they are identical, direct copy.

In that case maybe let's keep it that way and not make any changes, just the name change?

bobleesj · 2024-11-10T15:53:45Z

@sbillinge Ready for review, a few comments added via inline. Thank you.

bobleesj · 2024-11-10T15:54:36Z

src/diffpy/utils/scattering_objects/diffraction_object.py

File name has been changed from diffraction_objects to diffraction_object which carries DiffractionObject

why is the filename changed? This would lead to an API change if we add an additional diffraction object in the future.

using one class per file seems to be a modular approach. And considering this file is located under the scattering_objects directory, we could add additional classes as separate files.

doesn't this just add useless typing though?

from diffpy.utils.scattering_objects.diffraction_object import DiffractionObject from diffpy.utils.scattering_objects.pdf_object import PdfObject ...

The point of the current structure is that all diffraction objects go in the diffraction_objects module. What is the advantage of putting one per module?

For the useless typing, if we could utilize relative import, would this be a good solution below?

from diffpy.utils.scattering import DiffractionObject from diffpy.utils.scattering import PdfObject

also not sure if we want to call it as "Object" since we are utilizing a class and object is something we make with the class..

from diffpy.utils.scattering import Diffraction from diffpy.utils.scattering import Pdf

This a bit more feels like pythonic?

This doesn't quite convey what it does though. Diffraction is a method, a DiffractionObject is the result of a diffraction experiment. We could call it DiffractionDataset or something. I kind of like DiffractionObject though.

I thought the task was to bring the current naming more into PEP8 standard, not to do a refactor?

sbillinge

few comments. Also, please check if there were tests for Diffraction_object. If there were, we should leave them until Diffraction_object is removed.

sbillinge · 2024-11-13T13:03:30Z

src/diffpy/utils/scattering_objects/diffraction_object.py

why is the filename changed? This would lead to an API change if we add an additional diffraction object in the future.

sbillinge · 2024-11-13T13:04:59Z

src/diffpy/utils/scattering_objects/diffraction_object.py

+class DiffractionObject:
+    """
+    A class to manipulate diffraction data, supporting basic arithmetic operations and
+    conversions between different diffraction data metrics such as Q, two-theta, and d-spacing.


change metrics here to independent variables?

sbillinge · 2024-11-13T13:06:14Z

src/diffpy/utils/scattering_objects/diffraction_object.py

+
+    Parameters
+    ----------
+    name : str, optional


What is the difference between a parameter and an attribute here?

Also, per numpy standards, desriptions always start with "The" so wavelength would be "The wavelength of the radiation used in the diffraction experiment. Defaults to None." which, slightly magically, makes everything much more readable. Please change everywhere.

Fixed - added "The" in the front. Ref: https://sphinxcontrib-napoleon.readthedocs.io/en/latest/example_numpy.html

What is the difference between a parameter and an attribute here?

Fixed. Consistent now.

sbillinge · 2024-11-13T13:09:18Z

src/diffpy/utils/scattering_objects/diffraction_object.py

+    scat_quantity : str
+        Description of the type of scattering data stored.
+    on_q : list of numpy.ndarray
+        A list containing two numpy arrays: [Q values, Intensities].


The "the" constructions helps with these docstrings here, because we need a short statement of what this is, which is missing. In this case it could be "The diffraction intensities on a grid of Q" followed by something along the ines you already put (i.e., high level view followed by pertinent details).

sbillinge · 2024-11-13T13:11:01Z

src/diffpy/utils/scattering_objects/diffraction_object.py

+
+    def set_qs_from_range(self, begin_q, end_q, step_size=None, n_steps=None):
+        """
+        create an array of linear spaced Q-values


sbillinge · 2024-11-13T13:11:32Z

src/diffpy/utils/scattering_objects/diffraction_object.py

+        ----------
+        begin_q float
+          the beginning angle
+        end_q float


I think we usually have colons here end_q: float

bobleesj · 2024-11-13T14:21:29Z

few comments. Also, please check if there were tests for Diffraction_object. If there were, we should leave them until Diffraction_object is removed.

Thanks for the feedback I will address each in the afternoon.

sbillinge

pls see discussion comments.

sbillinge · 2024-11-13T15:48:02Z

src/diffpy/utils/scattering_objects/diffraction_object.py

doesn't this just add useless typing though?

from diffpy.utils.scattering_objects.diffraction_object import DiffractionObject from diffpy.utils.scattering_objects.pdf_object import PdfObject ...

The point of the current structure is that all diffraction objects go in the diffraction_objects module. What is the advantage of putting one per module?

sbillinge · 2024-11-13T15:49:37Z

src/diffpy/utils/scattering_objects/diffraction_object.py

+
+
+class DiffractionObject:
+    """


I see. So this is a direct copy-paste to duplicate it and then change the name, so we have both and they are identical to each other?

bobleesj

@sbillinge ready for review again. Thank you.

sbillinge

Please see comments. I think we are trying to do a bit too much of a refactor here. I suggest we just do the name-change and the deprecation warnings.

If we want to remove extra typing we could simply put DiffractionObject into scattering_objects.py module. This might make more sense.

In the future I expect we might have a PdfObject, but also others in principle, like XanesObject, RamanObject, and so on.

sbillinge · 2024-11-13T18:43:55Z

src/diffpy/utils/scattering_objects/diffraction_object.py

This doesn't quite convey what it does though. Diffraction is a method, a DiffractionObject is the result of a diffraction experiment. We could call it DiffractionDataset or something. I kind of like DiffractionObject though.

I thought the task was to bring the current naming more into PEP8 standard, not to do a refactor?

sbillinge · 2024-11-13T18:44:28Z

src/diffpy/utils/scattering_objects/diffraction_object.py

+
+
+class DiffractionObject:
+    """


In that case maybe let's keep it that way and not make any changes, just the name change?

bobleesj · 2024-11-14T16:18:54Z

Please see comments. I think we are trying to do a bit too much of a refactor here. I suggest we just do the name-change and the deprecation warnings.

If we want to remove extra typing we could simply put DiffractionObject into scattering_objects.py module. This might make more sense.

In the future I expect we might have a PdfObject, but also others in principle, like XanesObject, RamanObject, and so on.

Yes, too much refactoring I agree (good discussions still It think). I created a new PR below, just focused on changing the name and deprecating it.

#172

bobleesj added 2 commits November 9, 2024 12:26

Diffraction_object deprecation warning

25a089e

apply pre-commit

558cc1e

bobleesj commented Nov 9, 2024

View reviewed changes

bobleesj marked this pull request as ready for review November 9, 2024 17:31

bobleesj mentioned this pull request Nov 10, 2024

3.6.0 release issues overview #143

Open

12 tasks

bobleesj and others added 6 commits November 10, 2024 10:28

Include the word deprecated in the warning msg

6e9b465

Create a new object named DiffractionObject

2b0b112

Add news and a new test file for DiffractionObject

0161a8b

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

da60a89

Remove the unused warnings in DiffractionObject

71c9f99

:wq

dca3e23

erge branch 'deprecate' of https://github.com/bobleesj/diffpy.utils into deprecate

bobleesj changed the title ~~Diffraction_object deprecation warning - discussions needed~~ Deprecate Diffraction_object, rename to DiffractionObject Nov 10, 2024

bobleesj added 2 commits November 10, 2024 10:45

Remove methods in docstring

86838de

Apply pre-commit

7c73140

bobleesj commented Nov 10, 2024

View reviewed changes

sbillinge reviewed Nov 13, 2024

View reviewed changes

bobleesj added 2 commits November 13, 2024 12:58

Add The in front of each parameter description in docstring

16b8bbe

Add linear to linearly spaced...

ca872c0

bobleesj commented Nov 13, 2024

View reviewed changes

sbillinge reviewed Nov 13, 2024

View reviewed changes

bobleesj closed this Nov 14, 2024

bobleesj mentioned this pull request Nov 14, 2024

Deprecate Diffraction_object, rename to DiffractionObject (new clean PR) #172

Merged

Deprecate Diffraction_object, rename to DiffractionObject #165

Deprecate Diffraction_object, rename to DiffractionObject #165

Conversation

bobleesj commented Nov 9, 2024

bobleesj Nov 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 9, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobleesj left a comment

Choose a reason for hiding this comment

sbillinge commented Nov 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobleesj Nov 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobleesj commented Nov 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobleesj Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbillinge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobleesj commented Nov 13, 2024

sbillinge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobleesj left a comment

Choose a reason for hiding this comment

sbillinge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobleesj commented Nov 14, 2024

Deprecate `Diffraction_object`, rename to `DiffractionObject` #165

Deprecate `Diffraction_object`, rename to `DiffractionObject` #165

bobleesj Nov 9, 2024 •

edited

Loading

codecov bot commented Nov 9, 2024 •

edited

Loading

bobleesj Nov 10, 2024 •

edited

Loading

bobleesj Nov 13, 2024 •

edited

Loading