Skip to content

Rework ml model serialization #2773

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.

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

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from
Draft

Conversation

LukasBeiske
Copy link
Contributor

@LukasBeiske LukasBeiske commented Jun 12, 2025

This is a first rework of how we save our ml models. To move away from pickle completely some more changes are still needed, but this fixes #2720 and supersedes #2724.

TODO:

  • Tests
  • What kind of metadata do we want to save in the output?

TODO to move away from pickle -> Maybe as a follow up PR:

@LukasBeiske LukasBeiske requested review from maxnoe and Hckjs June 12, 2025 14:00
@maxnoe
Copy link
Member

maxnoe commented Jun 12, 2025

but this fixes #2720 which is needed for #2731

Why does adding a new combiner depend on the possibility to change the combiner between training and application?

Co-authored-by: Maximilian Linhoff <[email protected]>
@@ -386,6 +421,53 @@ def _predict_score(self, key, table):
def _get_positive_index(self, key):
return np.nonzero(self._models[key].classes_ == self.positive_class)[0][0]

def write(self, path, meta={}, overwrite=False):
"""
Save a dictionary using joblib-pickle, which contains all
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I started this with the intention of completely removing pickle. I can also convert this to draft until I can finish this.

This comment has been minimized.

1 similar comment

This comment has been minimized.

@LukasBeiske
Copy link
Contributor Author

LukasBeiske commented Jun 12, 2025

but this fixes #2720 which is needed for #2731

Why does adding a new combiner depend on the possibility to change the combiner between training and application?

It does not really, my bad, but the changes from #2724 are included in #2731. I adjusted the comment above.

@LukasBeiske LukasBeiske marked this pull request as draft June 12, 2025 15:05
Copy link

Failed

  • 22.97% Duplicated Lines (%) on New Code (is greater than 3.00%)
  • 79.20% Coverage on New Code (is less than 80.00%)

Analysis Details

1 Issue

  • Bug 0 Bugs
  • Vulnerability 0 Vulnerabilities
  • Code Smell 1 Code Smell

Coverage and Duplications

  • Coverage 79.20% Coverage (94.20% Estimated after merge)
  • Duplications 22.97% Duplicated Code (0.80% Estimated after merge)

Project ID: cta-observatory_ctapipe_AY52EYhuvuGcMFidNyUs

View in SonarQube

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.

Config values not applied to loaded Reconstructor models in ApplyModels
2 participants