Skip to content

Conversation

@tdixon97
Copy link
Collaborator

@tdixon97 tdixon97 commented Sep 24, 2025

Still some things are ad-hoc. My general idea is to have the public API functions convert the units to some internal set to be used in processors.
What I am not sure of is if this will really get used with the procesing chain / config (@ManuelHu ?)

Warning: This PR contains quite some API changes to switch to the new conventions

@tdixon97 tdixon97 requested a review from ManuelHu September 24, 2025 13:38
@ManuelHu
Copy link
Contributor

What I am not sure of is if this will really get used with the procesing chain / config

not yet. the parameters from the ak arrays are not stored into the lgdo for now...

Also ak.Array(..., attrs=...) is wrong; these attrs cannot be retreived any more. There is no way to do that in one line of code without being ugly.

a = ak.Array(np.array([1,2,4]), attrs={"unit": "m"})
ak.parameters(a)  # {}


a = ak.Array(np.array([1,2,4]))
a = ak.with_parameter(a, "unit", "m")
ak.parameters(a)  # {'unit': 'm'}

@tdixon97
Copy link
Collaborator Author

I do not understand

>>> ak.Array([1,2,3],attrs={"units":"kev"}).attrs
{'units': 'kev'}

works nicely

@tdixon97
Copy link
Collaborator Author

tdixon97 commented Sep 24, 2025

I think this is now fixed, but we should think about having much testing of the units handling. Eg for every processor:

  • Is the right unit returned?
  • Are the units used correctly (and not eg. just discarded)?

This will take time

@codecov
Copy link

codecov bot commented Sep 24, 2025

Codecov Report

❌ Patch coverage is 96.55172% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 68.47%. Comparing base (bfb6178) to head (ca832a2).

Files with missing lines Patch % Lines
src/reboost/hpge/psd.py 93.75% 1 Missing ⚠️
src/reboost/units.py 85.71% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #121      +/-   ##
==========================================
+ Coverage   68.44%   68.47%   +0.03%     
==========================================
  Files          31       31              
  Lines        2637     2630       -7     
==========================================
- Hits         1805     1801       -4     
+ Misses        832      829       -3     

☔ 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.

@gipert
Copy link
Member

gipert commented Nov 1, 2025

I think this is good to go and can be merged now, any objection?

@ManuelHu
Copy link
Contributor

ManuelHu commented Nov 1, 2025 via email

@gipert
Copy link
Member

gipert commented Nov 1, 2025

Ok let's wait until we properly test this

@tdixon97
Copy link
Collaborator Author

tdixon97 commented Nov 1, 2025 via email

@gipert
Copy link
Member

gipert commented Nov 20, 2025

We should try to make some progress here...

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.

3 participants