Skip to content

d_to_q, q_to_d - #197

Merged
sbillinge merged 7 commits into
diffpy:mainfrom
yucongalicechen:qd
Dec 7, 2024
Merged

d_to_q, q_to_d#197
sbillinge merged 7 commits into
diffpy:mainfrom
yucongalicechen:qd

Conversation

@yucongalicechen

@yucongalicechenyucongalicechen commented Dec 5, 2024

Copy link
Copy Markdown
Contributor

closes#176
@sbillinge ready for review

yucongalicechen
yucongalicechen commented Dec 5, 2024
View reviewed changes
Comment threadsrc/diffpy/utils/transforms.py Outdated
yucongalicechen
yucongalicechen commented Dec 5, 2024
View reviewed changes
Comment threadtests/test_transforms.py Outdated
sbillinge
sbillinge reviewed Dec 6, 2024
View reviewed changes

@sbillingesbillinge left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please see comments

Comment threadsrc/diffpy/utils/transforms.py Outdated
Comment threadsrc/diffpy/utils/transforms.py Outdated
The array of :math:`q` values np.array([qs]).
The units for the q-values are the inverse of the units of the provided wavelength.
This is the correct format for loading into diffpy.utils.DiffractionOject.on_q
This is the correct format for loading into diffpy.utils.DiffractionObject.on_q.

@sbillingesbillingeDec 6, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

delete, as discussed.

Comment threadsrc/diffpy/utils/transforms.py Outdated
The array of :math:`d` values np.array([ds]).
"""
if 0 in q:
raise ValueError(invalid_input_emsg)

@sbillingesbillingeDec 6, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just make this np.inf but also give a warning message that the largest d is infinite and can't be plotted? Something like that?

Comment threadsrc/diffpy/utils/transforms.py Outdated
Comment threadsrc/diffpy/utils/transforms.py
Comment threadtests/test_transforms.py Outdated
@yucongalicechen

yucongalicechen commented Dec 6, 2024

Copy link
Copy Markdown
ContributorAuthor

@sbillinge updated. please review again.

@codecov

codecovBot commented Dec 6, 2024•
edited
Loading

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.17%. Comparing base (ed9fb03) to head (39f1c54).
Report is 18 commits behind head on main.

Additional details and impacted files
@@ Coverage Diff @@## main #197 +/- ##
==========================================
- Coverage 96.53% 94.17% -2.37% 
==========================================
Files 8 8 Lines 289 309 +20 ==========================================
+ Hits 279 291 +12 - Misses 10 18 +8
Files with missing linesCoverage Δ
tests/test_transforms.py100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

bobleesj
bobleesj reviewed Dec 6, 2024
View reviewed changes
Comment threadsrc/diffpy/utils/transforms.py Outdated
invalid_input_emsg = (
"Input values have resulted in an infinite output. Please ensure there are no zeros in the input."
)
inf_output_msg = "WARNING: The largest output is infinite and cannot be plotted."

@bobleesjbobleesjDec 6, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sbillinge out of curiosity,

for both error and warning messages, do we still want to maintain our convention of 1) reason for error, (2) what to do to fix it ?

@sbillingesbillingeDec 7, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tweaked this a bit and downgraded it to an INFO from a WARNING....don't want users to freak out....

@sbillinge
sbillinge merged commit e6827a6 into diffpy:mainDec 7, 2024
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sbillingesbillingesbillinge left review comments
+1 more reviewer
@bobleesjbobleesjbobleesj left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

diffraction objects implement d_to_q, q_to_d

3 participants

@yucongalicechen@sbillinge@bobleesj