Skip to content

Add bindings to ReflectionProfile methods#79

Open
ezatterin wants to merge 16 commits intodiffpy:mainfrom
ezatterin:add-refl-prof-wrappers
Open

Add bindings to ReflectionProfile methods#79
ezatterin wants to merge 16 commits intodiffpy:mainfrom
ezatterin:add-refl-prof-wrappers

Conversation

@ezatterin
Copy link
Copy Markdown

Summary of changes:

  • allow to build the library via pip install . outside of a conda env using sysconfig to find the location of the shared libs.

  • added python bindings for the ReflectionProfile public methods:

      GetProfile, GetFullProfileWidth, XMLOutput, XMLInput
    

Tested on Ubuntu 24.04 and Windows 11.

@vincefn
Copy link
Copy Markdown
Collaborator

vincefn commented Nov 7, 2025

Hi @ezatterin - thanks for the changes and the fix.

Could you add a unit test (for good measure) with the new functions ?

I think the PR also needs a News item according to the template, as explained in https://scikit-package.github.io/scikit-package/programming-guides/billinge-group-standards.html#news-item-practice, but I'll let @Tieqiong or @sbillinge comment on this sicne I'm not yet a practitioner.

@sbillinge
Copy link
Copy Markdown
Contributor

yes, thanks @ezatterin and @vincefn . The news will turn into the changelog at the next release.

@ezatterin ezatterin marked this pull request as draft November 12, 2025 14:48
@ezatterin ezatterin marked this pull request as ready for review November 21, 2025 10:25
@ezatterin
Copy link
Copy Markdown
Author

Finally made it 🚀 I had to add an additional wrapper to use the assignCrystVector helper function for ReflectionProfile.GetProfile() to accept python sequences as x input, instead of just CrystVector as in the original C++ implementation.

@sbillinge sbillinge added this to the 2026.1.1 milestone Nov 30, 2025
@sbillinge
Copy link
Copy Markdown
Contributor

Thanks @ezatterin We will review. I added this to the 2026.1.0 release milestone so hopefully it can go out in early Jan if we can merge it.

@sbillinge
Copy link
Copy Markdown
Contributor

@vincefn please can you check you are ok to merge this? We are moving things forward to the next release so it would be nice to have this in there.

@ezatterin do you think documentation is adequate for folks to know how to use this? Bear in mind that we auto-build API docs from the docstrings, so if these are just library capabilities that may be enough, but if there are interesting things to do with this we could put an example in the docs.

@vincefn
Copy link
Copy Markdown
Collaborator

vincefn commented Apr 7, 2026

This looks fine for me, even if I have not had the time to test all the details (notably the changes of the setup.py).

@sbillinge
Copy link
Copy Markdown
Contributor

@vincefn that is a good point. We should test the build with the new setup.py before merging this, though if it is passing CI, presumably it works as it runs all tests in a clean env on the PR.

I will wait for @ezatterin to comment and then merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants