Do not allow an empty instance of DiffractionObject - require `xarraysyarraysxtype`#228

bobleesj · 2024-12-14T06:18:16Z

Closes#227

…th, and xtype

codecov · 2024-12-14T06:33:54Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (7b6ee2e) to head (ba4b985).
Report is 36 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #228 +/- ## ========================================= Coverage 100.00% 100.00% ========================================= Files 8 8 Lines 372 380 +8 ========================================= + Hits 372 380 +8

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

src/diffpy/utils/diffraction_objects.py

bobleesj · 2024-12-14T06:37:03Z

src/diffpy/utils/diffraction_objects.py

+
+ def input_data(self, xarray, yarray, xtype, wavelength, scat_quantity="", name="", metadata={}):
+ """
+ Insert a new scattering quantity into the scattering object.


Improved docstrings, previous version:
f""" insert a new scattering quantity into the scattering object Parameters ---------- xarray array-like of floats the independent variable array yarray array-like of floats the dependent variable array xtype string the type of quantity for the independent variable from{*XQUANTITIES, } metadata, scat_quantity, name and wavelength are optional. They have the same meaning as in the constructor. Values will only be overwritten if non-empty values are passed. Returns ------- Nothing. Updates the object in place. """

If we make it private we may want to move the docstring to the constructor? Also, then change "insert a new scattering object blah blah, to instantiate a new scattering object or sthg.

tests/conftest.py

sbillinge

Please see inline. We may want to check it is ok for us to have things like empty string details instead of None that converts to empty string

src/diffpy/utils/diffraction_objects.py

sbillinge · 2024-12-14T08:09:29Z

src/diffpy/utils/diffraction_objects.py

+
+ def input_data(self, xarray, yarray, xtype, wavelength, scat_quantity="", name="", metadata={}):
+ """
+ Insert a new scattering quantity into the scattering object.


If we make it private we may want to move the docstring to the constructor? Also, then change "insert a new scattering object blah blah, to instantiate a new scattering object or sthg.

src/diffpy/utils/diffraction_objects.py

sbillinge · 2024-12-14T08:15:43Z

src/diffpy/utils/diffraction_objects.py

- self.input_data(xarray, yarray, xtype)
+ self._input_xtype = xtype
+ self._set_xarrays(xarray, xtype)
+ self._all_arrays[:, 0] = yarray


It would be cleaner of this was moved into _set_arrays unless there was a reason not to do that because it is reused somewhere or something

tests/conftest.py

sbillinge · 2024-12-14T08:19:43Z

tests/test_diffraction_objects.py

 "scat_quantity": "",
- "wavelength": None,
- "xtype": "",
+ "wavelength": 0.71,


Return wavelengths back to None or absent to test that behavior.

sbillinge · 2024-12-14T08:28:05Z

tests/test_diffraction_objects.py


 tc_params = [
- (
-{},


Where are these things now getting tested?

pls see test_init_invalid_args below

but I think these are valid aren't they?

@sbillinge I think these aren't valid since we DiffractionObject requires x, y, and xtype (current PR)

bobleesj · 2024-12-14T17:06:37Z

Please see inline. We may want to check it is ok for us to have things like empty string details instead of None that converts to empty string

@sbillinge Before making PRs, when would be beneficial to have none for xtype or scat_quantity?

My understanding is that we usually want to pass typed variables so that typecasting can be minimal.

Extra comment:

I can see that having wavelength=none makes sense since we don't want a float value associated when it's not provided.

sbillinge · 2024-12-14T17:23:31Z

Please see inline. We may want to check it is ok for us to have things like empty string details instead of None that converts to empty string
@sbillinge Before making PRs, when would be beneficial to have none for xtype or scat_quantity?

I can't think of a case where we want none for xtype. I think we require this, right? For scat_quantity, the tradeoff is between making these easier to use (and therefore encouraging adoption) vs. making them stricter so we collect better metadata. I think leaning in the direction of adoption may be a good starting point for that (though if they are adopted, we may regret that later). That is the UC for a None default for scat_quantity: lazy users....

My understanding is that we usually want to pass typed variables so that typecasting can be minimal.

The issue is with setting mutable objects as defaults. In that case you should pass None and then you have logic like, if thing is None: thing = [] or whatever. I was being a bit too conservative about the empty strings. i was thinking they are mutable since they can act like lists, but I checked and they are not subject to this issue. So we could directly set empty strings (and of course floats and ints) as defaults.

Extra comment:
I can see that having wavelength=none makes sense since we don't want a float value associated when it's not provided.

yes, in this case None is different from 0. We want None here.

bobleesj · 2024-12-14T18:44:28Z

src/diffpy/utils/diffraction_objects.py

+ self.metadata = metadata
+ self.name = name
+ self._input_xtype = xtype
+ self._set_arrays(xarray, xtype, yarray)


as discussed - private func

src/diffpy/utils/diffraction_objects.py

tests/test_diffraction_objects.py

bobleesj

@sbillinge This is ready for review - pls see in-line comments..

bobleesj · 2024-12-14T19:08:45Z

(Just added news)

bobleesj

@sbillinge re-revisiting after resolving a conftest.py conflict. and ready for review.

sbillinge

I reviewed this yesterday or so but it looks as if I forgot to submit the review.....sorry.

src/diffpy/utils/diffraction_objects.py

sbillinge · 2024-12-14T19:37:55Z

src/diffpy/utils/diffraction_objects.py

+ >>> x = np.array([0.12, 0.24, 0.31, 0.4]) # independent variable (e.g., q)
+ >>> y = np.array([10, 20, 40, 60]) # intensity values
+ >>> metadata ={
+ ... "package_info":{"version": "3.6.0"}


this could only have been come up with by a programmer. Could we instead have an example of what we are after from materials scientists. Model some good behavior. Something like metadata ={'sample': "rock salt from the beach", "composition": "NaCl", "temperature": "300 K,", "experimenters": "Phill, Sally" }

src/diffpy/utils/diffraction_objects.py

sbillinge · 2024-12-14T19:41:15Z

src/diffpy/utils/diffraction_objects.py

 return i

- def _set_xarrays(self, xarray, xtype):
+ def _set_arrays(self, xarray, xtype, yarray):


maybe we should stay consistent...x, y, xtype order?

tests/test_diffraction_objects.py

sbillinge · 2024-12-14T19:44:41Z

tests/test_diffraction_objects.py


 tc_params = [
- (
-{},


but I think these are valid aren't they?

src/diffpy/utils/diffraction_objects.py

bobleesj · 2024-12-16T02:31:39Z

@sbillinge ready for review!

sbillinge · 2024-12-16T10:51:24Z

There is a lot of good stuff on here and this is really getting wrestled into shape in my mind so I want to merge this (and will). There were two things left to do that I will mae issues for. Thanks for your work on this. It is really great!

bobleesj added 8 commits December 14, 2024 01:09

Add do_minimal setup to conftest
92a2c4c

Refactor and remove input_data func
56541e5

Re-organize test with required parameters of xarray, yarray, waveleng…
1144d1f
…th, and xtype

Add input public data back to init func
e7b4dc2

Remove print diff function in pytest for debugging
6dc2738

Update input_data docstrings for clarity
50b7d0d

Add quotation marks around q, tth, d in docstrings
291cc89

Use empty string for default scattering quantity
b8a9adc

Final cleanup on input_data docstrings
e3c5208

bobleesj commented Dec 14, 2024
View reviewed changes

src/diffpy/utils/diffraction_objects.py Outdated Show resolvedHide resolved

bobleesj commented Dec 14, 2024
View reviewed changes

tests/conftest.pyShow resolvedHide resolved

bobleesj mentioned this pull request Dec 14, 2024
Do not allow an empty instance of DiffractionObject - init requires xarray, yarray, xtype#227
Closed

sbillinge reviewed Dec 14, 2024
View reviewed changes

bobleesj mentioned this pull request Dec 14, 2024
Refactor test_on_xtype() using @pytest.mark.parametrize and conftest.py#229
Closed

Merge branch 'main' into no-empty-object
db89596

bobleesj changed the title ~~Do not allow an empty instance of DiffractionObject~~Do not allow an empty instance of DiffractionObject - require xarraysyarraysxtypeDec 14, 2024

bobleesjand others added 4 commits December 14, 2024 13:41

make wavelength optional, test invalid init args
de6f12d

Add wavelength user behavior back
7ccebe8

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

Restore wavelenght none in one of the test cases
d1f767b

bobleesj commented Dec 14, 2024
View reviewed changes

src/diffpy/utils/diffraction_objects.pyShow resolvedHide resolved

Fix typo in docstrings
afe814b

bobleesj commented Dec 14, 2024
View reviewed changes

src/diffpy/utils/diffraction_objects.pyShow resolvedHide resolved

bobleesj commented Dec 14, 2024
View reviewed changes

tests/test_diffraction_objects.pyShow resolvedHide resolved

Use refined param names for pytest parametrize
46c7be8

bobleesj commented Dec 14, 2024
View reviewed changes

bobleesj marked this pull request as ready for review December 14, 2024 19:01

Add news for requires 3 input parameters of , ,
e285214

bobleesj mentioned this pull request Dec 14, 2024
Refactor test-on-xtype test function #231
Merged

bobleesjand others added 2 commits December 15, 2024 14:05

Merge branch 'main' into no-empty-object
4f54439

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

bobleesj commented Dec 15, 2024
View reviewed changes

bobleesj mentioned this pull request Dec 15, 2024
docs: Double-check and refine docstrings before 3.6.0 release #239
Closed

sbillinge reviewed Dec 16, 2024
View reviewed changes

bobleesjand others added 7 commits December 15, 2024 21:12

Add metadata data by materials scientist
8693153

Add name example to diffraction object example
e68b59e

Order x, y, xtype for _set_arrays
235f624

Remove 0.71 harded coded wavelength
88ea61b

Add wavelength of 0.711 and 0.71 back
6b20c1a

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

Final setup of wavelenth value comparison
aa1af39

bobleesj commented Dec 16, 2024
View reviewed changes

src/diffpy/utils/diffraction_objects.pyShow resolvedHide resolved

sbillinge added 3 commits December 16, 2024 05:36

news tweak
d09d7fb

tweak name example
0e362c1

tweak array length error message
c142240

sbillinge approved these changes Dec 16, 2024
View reviewed changes

sbillingeand others added 3 commits December 16, 2024 05:53

fix missing quote
24b1bbc

fix error message check
2c51af1

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

sbillinge merged commit da32ae3 into diffpy:mainDec 16, 2024
2 checks passed

bobleesj deleted the no-empty-object branch December 16, 2024 11:24

bobleesj mentioned this pull request Dec 26, 2024
revisit the invalid test DiffractionObject #241
Closed

Do not allow an empty instance of DiffractionObject - require xarraysyarraysxtype#228

Do not allow an empty instance of DiffractionObject - require xarraysyarraysxtype#228

Uh oh!

Conversation

bobleesj commented Dec 14, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecovbot commented Dec 14, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

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!

Uh oh!

sbillinge left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

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!

bobleesj commented Dec 14, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sbillinge commented Dec 14, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bobleesj left a comment • 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!

bobleesj commented Dec 14, 2024

Uh oh!

bobleesj left a comment

Choose a reason for hiding this comment

Uh oh!

sbillinge left a comment

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!

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!

Do not allow an empty instance of DiffractionObject - require `xarraysyarraysxtype`#228

Do not allow an empty instance of DiffractionObject - require `xarraysyarraysxtype`#228

bobleesj commented Dec 14, 2024•
edited
Loading

codecovbot commented Dec 14, 2024•
edited
Loading

bobleesj commented Dec 14, 2024•
edited
Loading

bobleesj left a comment •
edited
Loading