Skip to content

Re-organize data reduction - #371

Merged
ArneBouillon merged 5 commits into
orad/migrate-data-proc-to-newfrom
ab/data-processors-more-data
Jul 9, 2025
Merged

Re-organize data reduction#371
ArneBouillon merged 5 commits into
orad/migrate-data-proc-to-newfrom
ab/data-processors-more-data

Conversation

@ArneBouillon

@ArneBouillon ArneBouillon commented Jul 8, 2025

Copy link
Copy Markdown
Collaborator

This PR makes the following changes:

  • The orig2zscore and zscore2orig functions, which were unused, are removed.
  • The individual data processors are moved to their own files.
  • Decorrelater is renamed to Decorrelator
  • Instead of exporting all subroutines related to data processing, I now only export create_encoder_schedule, initialize_and_encode_with_schedule!, encode_with_schedule, and decode_with_schedule. My rationale for this is twofold.
    • It makes clear that these are the APIs that should be used in practice. The internal functions now start with an underscore (can be removed! just an idea) and can, if necessary, still be used outside the package as, e.g., CES.Utilities._initialize_and_encode_data!.
    • It allows the internal subroutines to implement only the methods necessary for our API functions, reducing the number of methods we need to define and maintain.
  • Partially thanks to the above change, I could remove the extract_data functions.

I also started working towards LikelihoodInformed processors by passing both structure matrices to the initialization code of PDCPs. I didn't pass the observation yet, since I'm still looking for a clean way to add this to the pipeline.

@ArneBouillon
ArneBouillon requested a review from odunbar July 8, 2025 01:44

@odunbar odunbar left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Much cleaner!

I'm very glad that we will remove the anonymous functions with these changes. I will just comment now, and approve on failed runtests. These seem almost entirely based on functions that have been removed and are tested.

You may also need to check that there is not entry for the normalizations z2orig... etc. in docs/src/API/Utilities.jl or docs may have issues.

@odunbar

odunbar commented Jul 8, 2025

Copy link
Copy Markdown
Member

I have addressed some of your comments in the PR so there may be small conflicts - I will refrain from further changes until this is merged. (and will work on a Docs page)

@ArneBouillon
ArneBouillon force-pushed the ab/data-processors-more-data branch from 5388a99 to 6de2813 Compare July 9, 2025 18:15
@ArneBouillon
ArneBouillon force-pushed the ab/data-processors-more-data branch 2 times, most recently from 8c179b0 to c8b927b Compare July 9, 2025 19:20
@ArneBouillon
ArneBouillon force-pushed the ab/data-processors-more-data branch from c8b927b to 8d33f8f Compare July 9, 2025 19:26
@codecov

codecov Bot commented Jul 9, 2025

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 97.24771% with 6 lines in your changes missing coverage. Please review.

Project coverage is 91.12%. Comparing base (7ad963c) to head (8d33f8f).
Report is 6 commits behind head on orad/migrate-data-proc-to-new.

Files with missing lines Patch % Lines
src/Utilities.jl 95.55% 2 Missing ⚠️
src/Utilities/canonical_correlation.jl 96.87% 2 Missing ⚠️
src/Utilities/decorrelator.jl 96.66% 2 Missing ⚠️
Additional details and impacted files
@@                        Coverage Diff                        @@
##           orad/migrate-data-proc-to-new     #371      +/-   ##
=================================================================
+ Coverage                          91.08%   91.12%   +0.03%     
=================================================================
  Files                                  7       10       +3     
  Lines                               1560     1544      -16     
=================================================================
- Hits                                1421     1407      -14     
+ Misses                               139      137       -2     

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

@odunbar odunbar left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM!

@ArneBouillon
ArneBouillon merged commit 3a2419e into orad/migrate-data-proc-to-new Jul 9, 2025
9 checks passed
@ArneBouillon
ArneBouillon deleted the ab/data-processors-more-data branch July 9, 2025 21:03
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