Skip to content

DRAFT: Fix: cache the compute ancillary params at indentation level#28

Open
PinkShnack wants to merge 5 commits intoAFM-analysis:masterfrom
PinkShnack:27_bugfix_ancill_comp_caching
Open

DRAFT: Fix: cache the compute ancillary params at indentation level#28
PinkShnack wants to merge 5 commits intoAFM-analysis:masterfrom
PinkShnack:27_bugfix_ancill_comp_caching

Conversation

@PinkShnack
Copy link

@PinkShnack PinkShnack commented Feb 10, 2026

As mentioned in #27, the ancillary param computation is currently very slow for extension models that require a model for the initial fit (e.g. KVM Hertz model).

This PR tries to fix that by caching the ancillary params at the Indentation (curve) level per fit. After the fit is complete, the cache is cleared for the next curve.

Perhaps we need a specific test for this with the KVM model? Thoughts?

Todo

  • make sure TODO note in core.py is removed if relevant
  • add ancillary test that actaully calculates ancillaries
  • confirm nanite and pyjibe give same ancillary results
    • They are very very close, and I reckon that pyjibe is doing something slightly different by default that I haven't found out yet. For example, the Emod for the test dataset is 10912 for pyjibe, and ~10890 for nanite.
  • ensure that pyjibe gives expected results with new cache system in nanite.

fix KVM model for numpy version 2 (put in separate PR)

@codecov
Copy link

codecov bot commented Feb 10, 2026

Welcome to Codecov 🎉

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

Thanks for integrating Codecov - We've got you covered ☂️

@PinkShnack PinkShnack marked this pull request as draft February 10, 2026 13:14
@paulmueller
Copy link
Member

test_fit_ancillary.py uses a fake model with ancillaries. You can add a test there for caching (no need to include the KVM model AFAIK.

I would initialize the cache with None and invalidate it before the fit is actually called (to prevent possible race conditions).

@PinkShnack
Copy link
Author

I added a test. I also checked to see how it looked with the KVM model locally. Turns out my nanite was using numpy 2 and therefore the KVM model extension failed due to a numpy np.argwhere error. Where can I fix the KVM model python code? (where is the code kept?).

@paulmueller
Copy link
Member

I added a test. I also checked to see how it looked with the KVM model locally. Turns out my nanite was using numpy 2 and therefore the KVM model extension failed due to a numpy np.argwhere error. Where can I fix the KVM model python code? (where is the code kept?).

I think it is supplementary material to the paper. Not sure whether there is a public repository for it. We could add a curated list of external model as a repository to AFM-analysis?

@PinkShnack
Copy link
Author

Yes, I wikll discuss with Shada, data is here: https://data.mendeley.com/datasets/c2gccnfkgd/2

Separately, I am trying to create a pyjibe GUI unit test that compares pyjibe's standard "fit_all" with the nanite equivalent. It seems some of the params_initial, params_fitted and ancillary (max_indent) are slightly different between nanite and pyjibe. And also slightly different depending on the order of the run i.e., running the pyjibe code first or the nanite.

This could be due to pyjibe doing lots of things in the backgroudn (such as updating the UI and grabbing info back from the UI). The differences are small.

I "feel" like the on_curve_list, which is automatically run when opening a dataset, is doing some weird things, as it automatically runs with the default hertz model.

I will try to get more into the details of why they are differing. I'm not expecting you to know the reason why they differ (though if you did, that's be great). I'm just typing out loud.

@PinkShnack
Copy link
Author

I think that we can merge this as is. The ancillary params are being cached, which speeds up pyjibe considerably with the KVM model.

The slight differences between nanite and pyjibe when using KVM should be fixed elsewhere imo.

@PinkShnack PinkShnack marked this pull request as ready for review February 23, 2026 10:14
@paulmueller
Copy link
Member

The way it is written now, the two new variables _anc_cache and _anc_valid could be consolidated into one variable. No need to check _anc_valid when _anc_cache contains a cache value or is None otherwise.

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