Skip to content

fix: fitall button and clicking per file should output identical results#51

Draft
PinkShnack wants to merge 6 commits into
AFM-analysis:masterfrom
PinkShnack:47_fix_fitall_ancillary
Draft

fix: fitall button and clicking per file should output identical results#51
PinkShnack wants to merge 6 commits into
AFM-analysis:masterfrom
PinkShnack:47_fix_fitall_ancillary

Conversation

@PinkShnack

@PinkShnack PinkShnack commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Issue from #47

Todo:

  • update changelog
  • make sure outputs are the same for normal and KVM models
  • make sure the output in the tsv file is the same

@codecov

codecov Bot commented Jun 23, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 67.85%. Comparing base (505a7bc) to head (1d18d3e).

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #51      +/-   ##
==========================================
+ Coverage   67.79%   67.85%   +0.06%     
==========================================
  Files          36       36              
  Lines        2816     2822       +6     
==========================================
+ Hits         1909     1915       +6     
  Misses        907      907              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@PinkShnack PinkShnack force-pushed the 47_fix_fitall_ancillary branch from 0d8962d to 5298a39 Compare June 23, 2026 11:34
@PinkShnack PinkShnack marked this pull request as ready for review June 23, 2026 11:53
@PinkShnack PinkShnack requested a review from paulmueller June 23, 2026 11:55
@PinkShnack

Copy link
Copy Markdown
Contributor Author

Hey @paulmueller: Stephanie and Jana need some changes. Please especially look at the readme section regarding use of AI. Thoughts on that?

@PinkShnack PinkShnack marked this pull request as draft June 24, 2026 12:02
@PinkShnack

Copy link
Copy Markdown
Contributor Author

When trying to compare whether the outputted TSV file is identical, three small bugs were found and corrected (by claude). They might be the fix to the issues we had with KVM and caching. More tests are needed. Here are the changes from claude:

  • 1 (fit_update_parameters, tab_fit.py:358): When switching models, the old params_initial (e.g., Hertz's "E" key) was passed to the new model's get_parm_name(), which threw a KeyError. Fix: detect model change via prev_model_key and fall back to fresh fit_parameters() when the model changes.

  • 2 (fit_update_parameters, tab_fit.py:363): Nanite's _anc_cache gets set to {'max_indent': ...} after the first (Hertz) fit. When the model switches to KVM, anc_update_parameters calls get_ancillary_parameters() which returns this stale Hertz-only cache — so anc_used = [], the ancillary table stays empty, and time_ind is never applied to itab. Fix: clear fdist._anc_cache when the model changes.

  • 3 (anc_update_parameters, tab_fit.py:120): Setting itab.item(rr, 1).setText(value) inside anc_update_parameters (without signal blocking) fired on_params_init → recursive anc_update_parameters → nested atab.blockSignals(False) calls unblocked atab prematurely, causing chaotic state. Fix: wrap the itab.item.setText() call in itab.blockSignals(True/False).

@paulmueller

Copy link
Copy Markdown
Member

It would take me a bit longer to get back into the code and understand the changes.
Have you understood and verified the proposed changes?
I think it is not necessary to mention the use of AI in the readme. You can mention the use of AI in a commit (we can do that in the squash-merge-commit).
Ultimately, it is your commit and LLMs are just tools.

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