Skip to content

Comments

fix(dap): lmfit1d can pass any parameters with kwargs#707

Open
wyzula-jan wants to merge 5 commits intomainfrom
fix/dap-lmfit
Open

fix(dap): lmfit1d can pass any parameters with kwargs#707
wyzula-jan wants to merge 5 commits intomainfrom
fix/dap-lmfit

Conversation

@wyzula-jan
Copy link
Contributor

@wyzula-jan wyzula-jan commented Dec 15, 2025

Adaptation that LMFIT parameters kwargs can be passed for lmfit1d_service, required for bec-project/bec_widgets#987 ; requested by cSAXS

One can now pass multiple models for more complex fitting such as multiple Gaussians, fit background etc. Check bec-project/bec_widgets#987 where are examples and showcase of high level API in waveform.py

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR enhances the lmfit1d service to accept generic fitting parameters through a flexible kwargs-based API. The main improvement allows users to pass any valid model parameters via a parameters dictionary argument, with automatic validation and filtering of invalid parameter names.

Key Changes:

  • Added a new parameters kwarg to the configure method that accepts either dict or lmfit.Parameters objects
  • Implemented parameter validation logic that filters out invalid parameters for the selected model
  • Enhanced error handling and logging in the fit process with detailed parameter tracking

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 11 comments.

File Description
bec_server/bec_server/data_processing/lmfit1d_service.py Core implementation of flexible parameter handling, validation, and filtering logic; added error handling with logging for fit execution
bec_lib/bec_lib/lmfit_serializer.py Updated deserialize_param_object to handle both dict and Parameter object inputs; changed return type annotation from Parameter to Parameters
bec_server/tests/tests_data_processing/test_lmfit1d_service.py Added tests for generic parameter passing and validation filtering across different model types (GaussianModel and SineModel)
bec_lib/tests/test_lmfit_serializer.py Added test case to verify deserialization works with optional "name" field in parameter dictionaries

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@codecov
Copy link

codecov bot commented Dec 15, 2025

Codecov Report

❌ Patch coverage is 73.89381% with 59 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
...rver/bec_server/data_processing/lmfit1d_service.py 73.27% 39 Missing and 19 partials ⚠️
bec_lib/bec_lib/lmfit_serializer.py 88.88% 0 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

@wyzula-jan wyzula-jan requested a review from wakonig January 28, 2026 16:25
@wyzula-jan wyzula-jan marked this pull request as draft January 28, 2026 22:07
@wyzula-jan wyzula-jan force-pushed the fix/dap-lmfit branch 2 times, most recently from fb0513b to c595215 Compare January 28, 2026 22:32
@wyzula-jan wyzula-jan marked this pull request as ready for review January 28, 2026 22:55
Copy link
Member

@wakonig wakonig left a comment

Choose a reason for hiding this comment

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

In general, it looks good to me. I would suggest to refactor the long methods a bit to improve the readability and avoid the deep nesting.

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.

2 participants