Skip to content

ENH: Stop printing false positive differences when logging cached nodes #3376

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

Merged
merged 7 commits into from
Oct 13, 2021

Conversation

oesteban
Copy link
Contributor

@oesteban oesteban commented Sep 17, 2021

The function calculating changed input fields did not preempt the problem of comparing tuples vs. lists in most of the cases. This PR simplifies the logic of the dict_diff accessory function, and fixes the issue - resulting in way shorter indices of changed inputs (esp. when an interface has several inputs and some of them contain long lists).

This issue translated in practice into showing long lists of unchanged values as if they had changed, when often, the changed input invalidating the cache would be some other field.

EDIT: Beyond the more robust implementation (former was kind of brittle), this PR is useful, e.g., when re-running fMRIPrep and large lists of files have changed (e.g., the inputs to an fslmerge node with a list of 800 input files).

@oesteban oesteban requested a review from effigies September 23, 2021 10:39
@oesteban
Copy link
Contributor Author

I don't think the failing test is related, right?

@effigies

This comment has been minimized.

@oesteban oesteban force-pushed the enh/less-verbose-diffs branch 2 times, most recently from 238d1c2 to a30018e Compare October 1, 2021 08:11
@effigies effigies force-pushed the enh/less-verbose-diffs branch from 5cd45ad to 90e2130 Compare October 13, 2021 18:15
@effigies
Copy link
Member

@oesteban Heads up that I rebased.

@codecov
Copy link

codecov bot commented Oct 13, 2021

Codecov Report

Merging #3376 (1beec59) into master (d8dbc6f) will increase coverage by 0.07%.
The diff coverage is 90.90%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3376      +/-   ##
==========================================
+ Coverage   64.88%   64.95%   +0.07%     
==========================================
  Files         307      307              
  Lines       40365    40365              
  Branches     5337     5339       +2     
==========================================
+ Hits        26189    26220      +31     
+ Misses      13081    13049      -32     
- Partials     1095     1096       +1     
Flag Coverage Δ
unittests 64.95% <90.90%> (+0.07%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
nipype/utils/misc.py 78.98% <90.90%> (+19.74%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d8dbc6f...1beec59. Read the comment docs.

@effigies effigies force-pushed the enh/less-verbose-diffs branch from 90e2130 to 6bd611f Compare October 13, 2021 19:06
@effigies effigies force-pushed the enh/less-verbose-diffs branch from 6bd611f to 1beec59 Compare October 13, 2021 19:16
@effigies effigies merged commit c0d450e into nipy:master Oct 13, 2021
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