Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Slightly better unit propagation #109

Merged
merged 6 commits into from
May 26, 2024
Merged

Slightly better unit propagation #109

merged 6 commits into from
May 26, 2024

Conversation

fjebaker
Copy link
Owner

No description provided.

fjebaker added 6 commits May 25, 2024 23:29
@fjebaker
feat(grouping): regroup with minimum counts
9838469
@fjebaker
fix!: refactor AbstractDataLayout as AbstractLayout
4de4a1f
@fjebaker
feat!: better support API
c2d4c62 
Changed the function `supports` to now return a list of the supported
layouts instead of having to define a new dispatch for each one.
@fjebaker
feat: add support_units for adding unit information to layouts
42ed291
@fjebaker
feat: add units API to AbstractDataset
ddcf631 
Add a specification of how the abstract dataset should indicate
preferred units per fitting statistic, so that different statistics can
be easily fit in different regimes.
@fjebaker
feat: units in fitting
5577b71 
Started adding the code that tracks that the model, data, and fit
statistic are used to change the units used when fitting. For example,
when using Cash stat, we want to fit in counts, but for ChiSquared we
want to fit in counts / (s * keV^-1), or similar.

There is a looming issue here related to type instability with the
units, since the units are basically compile time structs, but the way
we store them in e.g. Spectrum is as a union so they may easily be
modified.

This means we can't rely on the dataset units for codegen.

deprecated: normalize!
@fjebaker
Copy link
Owner Author

fjebaker commented May 26, 2024 •
edited
Loading

Just for sanity, old:

BenchmarkTools.Trial: 138 samples with 1 evaluation.
 Range (min … max):  34.311 ms … 40.633 ms  ┊ GC (min … max): 0.00% … 8.66%
 Time  (median):     36.498 ms              ┊ GC (median):    0.00%
 Time  (mean ± σ):   36.246 ms ±  1.079 ms  ┊ GC (mean ± σ):  0.53% ± 1.24%

       ▂              █▇▁█▄ ▃                                  
  ▆▄▄█▆█▇█▇▄▃▃▇▇▄▁▆▄█▃█████▄█▇▄▆▆▃▃▃▁▁▁▁▁▁▁▃▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▃ ▃
  34.3 ms         Histogram: frequency by time        40.5 ms <

 Memory estimate: 11.23 MiB, allocs estimate: 2733.

New:

BenchmarkTools.Trial: 139 samples with 1 evaluation.
 Range (min … max):  33.946 ms … 45.828 ms  ┊ GC (min … max): 0.00% … 10.15%
 Time  (median):     35.613 ms              ┊ GC (median):    0.00%
 Time  (mean ± σ):   36.007 ms ±  1.551 ms  ┊ GC (mean ± σ):  0.53% ±  1.62%

      ▂ ▄▅▇▂ █▃▃  ▂▂                                           
  ▅▁▅▅█▃████████▇███▆▇▇▆▅▅▅▅▆▁▆▅▁▃▅▃▁▁▁▁▃▃▁▁▃▁▃▁▃▁▁▁▁▁▃▁▁▁▁▁▃ ▃
  33.9 ms         Histogram: frequency by time        41.5 ms <

 Memory estimate: 11.24 MiB, allocs estimate: 2742.

The currently introduced changes incur no fitting performance impact.

The above benchmark is for the fit call in the XMM test from the test suite.

@fjebaker fjebaker merged commit 9101c90 into main May 26, 2024
1 check passed
@fjebaker fjebaker deleted the fergus/units branch May 26, 2024 02:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@fjebaker