Skip to content

Set prec field based on min value during introspection#64

Merged
GDYendell merged 7 commits intomainfrom
set_prec_field_based_on_min
May 15, 2025
Merged

Set prec field based on min value during introspection#64
GDYendell merged 7 commits intomainfrom
set_prec_field_based_on_min

Conversation

@shihab-dls
Copy link
Contributor

Fixes #63

Following #62, we are now be able to set the prec field of attributes by parsing their min value.

@shihab-dls shihab-dls requested a review from DominicOram April 29, 2025 10:14
@codecov
Copy link

codecov bot commented Apr 29, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.40%. Comparing base (840932c) to head (99fd3b2).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #64      +/-   ##
==========================================
+ Coverage   86.02%   86.40%   +0.37%     
==========================================
  Files           5        5              
  Lines         322      331       +9     
==========================================
+ Hits          277      286       +9     
  Misses         45       45              

☔ View full report in Codecov by Sentry.
📢 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.

Copy link

@DominicOram DominicOram left a comment

Choose a reason for hiding this comment

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

Looks good, thanks. Can we have some additional tests on if min comes back as not a float and maybe just parameterize a bunch of other potential values of min and what you would expect the prec to be

Copy link

@DominicOram DominicOram left a comment

Choose a reason for hiding this comment

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

Looks good, thank you!

Copy link

@DominicOram DominicOram left a comment

Choose a reason for hiding this comment

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

As found on the beamline you can get mins come in with 1e-7 notation . Can we have some tests on that?

@shihab-dls
Copy link
Contributor Author

Converting min to decimal representation (which we did in i03) (i.e., something like f"{:.f}) is neater as it handles scientific notation, but it pads with zeroes (default is 6 decimal places), which thus requires an upper bound for precision like f"{:.16f}. This introduces floating point errors (e.g., f"{0.1:.16f} == 0.1000000000000001), which result in an erroneous precision. So, I've just added a more explicit helper function instead.

@shihab-dls shihab-dls requested a review from DominicOram May 7, 2025 09:46
@shihab-dls shihab-dls requested a review from GDYendell May 13, 2025 15:24
Copy link

@DominicOram DominicOram left a comment

Choose a reason for hiding this comment

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

One minor comment in code, take it or leave it. Is there also a bug here where if the min comes back as positive e.g. 1e10 it will have a prec of 10? It's probably not a massive issue

@GDYendell GDYendell merged commit ac59b61 into main May 15, 2025
19 checks passed
@GDYendell GDYendell deleted the set_prec_field_based_on_min branch May 15, 2025 15:36
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.

Prec field left as default for all introspected datatypes.

3 participants