Skip to content

fix: allow matching nan values in annotations (e.g. when b_factors are set to nan)#714

Merged
padix-key merged 4 commits intobiotite-dev:mainfrom
Croydon-Brixton:fix/equal_nan_when_comparing_annotations
Dec 12, 2024
Merged

fix: allow matching nan values in annotations (e.g. when b_factors are set to nan)#714
padix-key merged 4 commits intobiotite-dev:mainfrom
Croydon-Brixton:fix/equal_nan_when_comparing_annotations

Conversation

@Croydon-Brixton
Copy link
Copy Markdown
Contributor

A tiny fix, to allow counting nan values as equal in annotations. This makes sense in cases where annotations are flagged as not available (e.g. b_factor = nan).

@codspeed-hq
Copy link
Copy Markdown

codspeed-hq bot commented Dec 9, 2024

CodSpeed Performance Report

Merging #714 will not alter performance

Comparing Croydon-Brixton:fix/equal_nan_when_comparing_annotations (7ff00ed) with main (05e40cb)

Summary

✅ 47 untouched benchmarks

Copy link
Copy Markdown
Member

@padix-key padix-key left a comment

Choose a reason for hiding this comment

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

Hi, thanks for bringing this up. In my opinion you are right: When two AtomArrayhave NaN annotations at the same position, as a user I would expect that they are still the same. I would even suggest to be more strict and enforce this behavior (and remove the parameter), but I do not have a strong opinion here.

I only left a small nit, otherwise this looks good to me.

@Croydon-Brixton
Copy link
Copy Markdown
Contributor Author

Thank you for the quick review @padix-key , I've updated the docstring as you suggested & fixed ruff. Good to merge from my side.

@padix-key padix-key merged commit 0acb143 into biotite-dev:main Dec 12, 2024
@Croydon-Brixton Croydon-Brixton deleted the fix/equal_nan_when_comparing_annotations branch January 27, 2025 13:44
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