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.

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

document the support for constraints #300

Merged
merged 19 commits into from
Dec 15, 2023
Merged

document the support for constraints #300

merged 19 commits into from
Dec 15, 2023

Conversation

prjemian
Copy link
Contributor

@prjemian prjemian commented Nov 10, 2023

@prjemian prjemian added this to the v1.1 milestone Nov 10, 2023
@prjemian prjemian self-assigned this Nov 10, 2023
@prjemian
Copy link
Contributor Author

During testing (and confirming user reports), the value and fit parameters have no effect on the forward solutions. This needs further testing.

Marking this PR as draft for now.

@prjemian prjemian marked this pull request as draft November 18, 2023 06:57
@prjemian
Copy link
Contributor Author

The diffractometer (engine) mode is used to hold an axis at a constant value for a forward(hkl) calculation. The Constraints value attribute:

Constant value to be used if held constant by forward mode.

@prjemian
Copy link
Contributor Author

This comment describes the meaning of fit, but not in the context of forward() computations.

- Crystal lattice refinement
  - with more than 2 reflections you can select which parameter must
    be fitted.

Here are the places in libhkl source where fit_get appears and this parameter might be used:

(base) prjemian@arf:~/.../picca/hkl$ git grep fit_get
gui/hkl-gui.c:          fit = hkl_parameter_fit_get(p);                         \
gui/hkl-gui.c:          gboolean fit = hkl_parameter_fit_get(p);                \
hkl.h:HKLAPI int hkl_parameter_fit_get(const HklParameter *self) HKL_ARG_NONNULL(1);
hkl/hkl-parameter.c: * hkl_parameter_fit_get:
hkl/hkl-parameter.c:int hkl_parameter_fit_get(const HklParameter *self)
tests/hkl-axis-t.c:     ok(TRUE == hkl_parameter_fit_get(axis), __func__);
tests/hkl-axis-t.c:     ok(TRUE == hkl_parameter_fit_get(axis), __func__);
tests/hkl-axis-t.c:     ok(TRUE == hkl_parameter_fit_get(copy), __func__);
tests/hkl-axis-t.c:     ok(TRUE == hkl_parameter_fit_get(copy), __func__);

Not apparent it is used anywhere in the computation of forward(hkl) solutions.

@prjemian
Copy link
Contributor Author

To avoid making a breaking change, by dropping this parameter altogether, consider these actions now:

  • deprecate its use
  • provide warning messages

@prjemian
Copy link
Contributor Author

Confirmed with Fred Picca that the fit parameter is not used when calculating angles from h,k,l:

This interface was developped for the sample parameters a/b/c/alpha/beta/gamma/ux/uy/uz in order to fit the UB martix.
The parameter interface define a fit_get/set and since the axes derived from this interface, the functions are available, but of no use.

@prjemian prjemian marked this pull request as ready for review November 21, 2023 04:39
@prjemian
Copy link
Contributor Author

@mrakitin @rodolakis @strempfer @padraic-shafer -- This is ready for review now.

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.

The updated documentation is helpful and clearer. Thanks for that!

(Btw, I skipped the SPEC docs in this review)

hkl/tests/test_diffract.py Outdated Show resolved Hide resolved
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.

Looks good!
I have a suggestion about the .ipynb files rendering, which can be addressed in a separate PR.

Copy link
Member

Choose a reason for hiding this comment

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

The notebooks like this one are already rendered, which makes it hard to review as there are huge image diffs there. I think we can use https://github.com/kynan/nbstripout and auto-render the notebooks during the sphinx docs building process using https://nbsphinx.readthedocs.io/en/0.9.3/.

@prjemian prjemian mentioned this pull request Dec 15, 2023
@prjemian prjemian merged commit 83236a1 into main Dec 15, 2023
8 checks passed
@prjemian prjemian deleted the 290-docs-constraints branch December 15, 2023 14:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: restore(configuration) should push constraints to stack Constraints stack needs documentation
3 participants