Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

avoid crash in add_reflections, add axes_(r,w,c) #309

Merged
merged 21 commits into from
Dec 13, 2023
Merged

avoid crash in add_reflections, add axes_(r,w,c) #309

merged 21 commits into from
Dec 13, 2023

Conversation

prjemian
Copy link
Contributor

@prjemian prjemian commented Dec 9, 2023 •
edited
Loading

@prjemian prjemian added the task label Dec 9, 2023
@prjemian prjemian added this to the v1.1 milestone Dec 9, 2023
@prjemian prjemian self-assigned this Dec 9, 2023
@prjemian prjemian marked this pull request as draft December 9, 2023 01:55
@prjemian prjemian marked this pull request as ready for review December 12, 2023 16:50
padraic-shafer
padraic-shafer reviewed Dec 12, 2023
View reviewed changes
Copy link
Contributor

@padraic-shafer padraic-shafer left a comment

Choose a reason for hiding this comment

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

I didn't have a chance to look at the tests yet. That might skew the comments I left. :)

hkl/engine.py Outdated Show resolved Hide resolved
hkl/sample.py Outdated Show resolved Hide resolved
hkl/sample.py Outdated Show resolved Hide resolved
hkl/sample.py Show resolved Hide resolved
padraic-shafer
padraic-shafer reviewed Dec 13, 2023
View reviewed changes
hkl/sample.py Outdated Show resolved Hide resolved
@prjemian @padraic-shafer
Update hkl/sample.py
045c575 
Co-authored-by: Padraic Shafer <76011594+padraic-shafer@users.noreply.github.com>
prjemian
prjemian commented Dec 13, 2023
View reviewed changes
hkl/sample.py Outdated Show resolved Hide resolved
padraic-shafer
padraic-shafer approved these changes Dec 13, 2023
View reviewed changes
Copy link
Contributor

@padraic-shafer padraic-shafer left a comment

Choose a reason for hiding this comment

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

I see that you're planning to add more tests.
Noting here that the tests so far look reasonable.

@prjemian
Copy link
Contributor Author

@padraic-shafer : I hardened the validation process for namedtuples. Previously, an unacceptable namedtuple instance was handled by isinstance(pos, (list, tuple)).

padraic-shafer
padraic-shafer approved these changes Dec 13, 2023
View reviewed changes
Copy link
Contributor

@padraic-shafer padraic-shafer left a comment

Choose a reason for hiding this comment

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

Latest updates look good.

@padraic-shafer
Copy link
Contributor

Although probably beyond what you have in mind for this PR, the extent and combination of validation tests suggest that hypothesis for property-based testing could be worthwhile in the long run.

@prjemian
Copy link
Contributor Author

That looks like a good addition for the v2.0 release. For now, I'm trying to keep this v1.1.0 release limited to the new geometries and some user-requested features & bug fixes. This is the last planned minor (semver: major.minor.patch) release before v2. Since the changes for v2.0 are planned as a complete overhaul, this might be too much for now.

mrakitin
mrakitin approved these changes Dec 13, 2023
View reviewed changes
Copy link
Member

@mrakitin mrakitin left a comment

Choose a reason for hiding this comment

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

Very detailed tests!
I spotted a couple of typos, it's good to go once fixed.

hkl/calc.py Outdated Show resolved Hide resolved
hkl/calc.py Outdated Show resolved Hide resolved
hkl/calc.py Outdated Show resolved Hide resolved
hkl/sample.py Outdated Show resolved Hide resolved
prjemian and others added 4 commits December 13, 2023 16:47
@prjemian @mrakitin
Update hkl/calc.py
dc4d519 
Co-authored-by: Max Rakitin <mrakitin@users.noreply.github.com>
@prjemian @mrakitin
Update hkl/calc.py
9f1d145 
Co-authored-by: Max Rakitin <mrakitin@users.noreply.github.com>
@prjemian @mrakitin
Update hkl/calc.py
342fdf8 
Co-authored-by: Max Rakitin <mrakitin@users.noreply.github.com>
@prjemian
MNT #309 another type annotation
58ecdbb
@prjemian
Copy link
Contributor Author

@padraic-shafer @mrakitin : Thanks for the reviews!

@prjemian prjemian merged commit 95be6c9 into main Dec 13, 2023
8 checks passed
@prjemian prjemian deleted the 307-add_reflections-crashes branch December 13, 2023 23:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mrakitin mrakitin mrakitin approved these changes

@padraic-shafer padraic-shafer padraic-shafer approved these changes

Assignees

@prjemian prjemian

Labels
task
Projects
None yet
Milestone
v1.1
Development

Successfully merging this pull request may close these issues.

Add properties: axes_r, axes_w, & axes_c BUG: add_reflections() crashes session when ...
3 participants
@prjemian @padraic-shafer @mrakitin