Skip to content

Single kernel call for volume and intensity#3963

Open
jellybean2004 wants to merge 5 commits into
SD_calc_reformatfrom
SD_remove_hardcode
Open

Single kernel call for volume and intensity#3963
jellybean2004 wants to merge 5 commits into
SD_calc_reformatfrom
SD_remove_hardcode

Conversation

@jellybean2004

Copy link
Copy Markdown
Member

Description

Removes hard-coded/duplicated volume math and ensures intensities/volumes come from the model kernel.

Replaces the previous per-bin DirectModel/analytic volume approach with a kernel-based single call.
Builds the sasmodels kernel (from model info) and calls call_Fq for each bin to retrieve Fsq and volume. Compute intensity per bin as intensity = (scale/volume) * Fsq + background and store per-bin volumes in self._volumes.

Fixes #3418

How Has This Been Tested?

Tested functionality -- same as before.

Review Checklist:

Documentation

  • There is nothing that needs documenting
  • Documentation changes are in this PR
  • There is an issue open for the documentation (link?)

Installers

  • There is a chance this will affect the installers, if so
    • Windows installer (GH artifact) has been tested (installed and worked)
    • MacOSX installer (GH artifact) has been tested (installed and worked)
    • Wheels installer (GH artifact) has been tested (installed and worked)

Licensing

  • The introduced changes comply with SasView license (BSD 3-Clause)

Copilot AI 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.

Pull request overview

This PR updates the size distribution model-matrix generation so that both per-bin intensity and volume are obtained via a single sasmodels kernel call, removing the prior duplicated/hard-coded volume math path.

Changes:

  • Replaces load_model/DirectModel usage with load_model_info + build_model and call_Fq in generate_model_matrix.
  • Computes per-bin intensity from returned kernel outputs and stores per-bin volumes on the class for later use.
Comments suppressed due to low confidence (1)

src/sas/sascalc/size_distribution/SizeDistribution.py:193

  • self._volumes is initialized twice in __init__ (once untyped, then again with a type annotation). This duplication is redundant and can confuse readers/type checkers; remove the earlier self._volumes = None assignment and keep the single typed initialization.
        self.model_matrix: np.ndarray | None = None
        self._volumes = None

        # Advanced parameters for MaxEnt
        self._iterMax: int = 5000
        self._skyBackground: float = 1e-6
        self._weightType: str = "dI"
        self._weightFactor: float = 1.0
        self._weightPercent: float = 1.0
        self._weights: np.ndarray | None = np.array(data.dy)

        self._bin_edges: np.ndarray = np.array([])
        self._binDiff: np.ndarray = np.array([])
        self._volumes: np.ndarray | None = None

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

Comment thread src/sas/sascalc/size_distribution/SizeDistribution.py Outdated
Comment thread src/sas/sascalc/size_distribution/SizeDistribution.py Outdated
Comment thread src/sas/sascalc/size_distribution/SizeDistribution.py Outdated
call_pars = p.copy()
_, Fsq, _, volume, volume_ratio = call_Fq(calculator, call_pars)

# Compute intensity using kernel convention: combined_scale = scale / shell_volume

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.

This is duplicating code from within sasmodels.kernel.Kernel.Iq. This isn't avoidable in the current implementation, but we should consider this use case when addressing sasmodels #182.

The idea is to return intermediate and derived values from the calculation back to the caller. The current hack to get $P(Q)$ and $S(Q)$ from P@S is that sasmodels.product sets up a kernel.results() function that returns a dictionary with keys P(Q), S(Q), volume, volume_ratio and radius_effective after calling kernel.Iq(). We could extend this to the base Kernel class in sasmodels. Then using call_kernel instead of call_Fq we can get the volume and volume ratio from calculator.results().

@jellybean2004

Copy link
Copy Markdown
Member Author

Hey @pkienzle! Thanks for your review! I have addressed your comments in my recent commit.

@jellybean2004 jellybean2004 linked an issue May 26, 2026 that may be closed by this pull request
@pkienzle

Copy link
Copy Markdown
Contributor

_volumes is never used. Is this preparation for a future change?

@jellybean2004

Copy link
Copy Markdown
Member Author

_volumes is never used. Is this preparation for a future change?

I somehow managed to miss one commit. The goal in the issue was to replace ellsipse_volume with self._volumes, and I have now done that.

@krzywon krzywon requested a review from pkienzle June 16, 2026 13:36
@pkienzle

Copy link
Copy Markdown
Contributor

Don't scale volume by volume ratio. Intensity scales with the volume fraction of the material in the shell. Volume ratio is for determining the average volume enclosed by the shape when computing structure factors. Since shape is fixed as ellipsoid then volume ratio is one and you won't see a problem. Maxent on vesicles would be a problem, which you can verify by comparing to maxent on a core-shell model.

With background fixed at zero and scale fixed at one you don't need to include them in the intensity calculation. It doesn't hurt, but it doesn't help either.

The model_matrix and _volumes attributes have the same status, either both private or both public.

Outside this PR there are a number of things that could change.

Linear systems don't need levenberg-marquardt. For background_fit you can use np.polyfit with deg=0|1 and w=1/σ.

PEP8 recommends short snake_case rather than CamelCase for module names, so size_distribution.py and maxent.py.

PEP8 recommends CamelCase for class names and snake_case for functions and methods, such as maxent.DecisionHelper.chi_now.

Shouldn't be using a builtin function like iter as a for loop index: for iter in range(IterMax): => for step in range(maxiter):

In class SizeDistribution, you don't need to use properties for scale, background, etc. with the setter assigning the internal value unchecked and the getter returning the value unaltered. Simple attributes work well for these.

The weight type setter in size distribution is suspect. It is calling update weights with default None for the full data, whereas the prep maxent call is using a trimmed q range for the data. Easiest if changing weight type does nothing special since the prep maxent call provides the data for weights.

More generally, the size distribution controls aren't going to change during analysis, so they should be set as arguments to the constructor rather than assigned as attributes.

SizeDistributionThread is accessing "private" sd._binDiff. If it is needed by the caller then the attribute shouldn't be tagged with a leading underscore.

@pkienzle

Copy link
Copy Markdown
Contributor

Please fix the merge conflicts and remove the volume ratio scaling. Code style can be put in as a separate issue if you want to minimize changes for this PR.

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.

Fix problem of hard coding in Size Distribution

3 participants