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.

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

Added a lazy result object. #1421

Merged
merged 10 commits into from
Jul 29, 2024
Merged

Added a lazy result object. #1421

merged 10 commits into from
Jul 29, 2024

Conversation

PaulJonasJost
Copy link
Collaborator

@PaulJonasJost PaulJonasJost commented Jun 25, 2024

Added a result object, that reads the results only if needed. Connected to #1405

Things to consider still

  • Do we want a casing limit, if yes how would that look like?
  • Should the lazy result actually be a subclass of the optimizer result?
  • Connected with the above, when printing the lazy result, all attributes are shown as None

@PaulJonasJost PaulJonasJost self-assigned this Jun 25, 2024
@codecov-commenter
Copy link

codecov-commenter commented Jun 25, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 73.20261% with 41 lines in your changes missing coverage. Please review.

Project coverage is 83.24%. Comparing base (e746456) to head (6f76f46).

Files Patch % Lines
pypesto/result/optimize.py 71.72% 41 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1421      +/-   ##
===========================================
- Coverage    83.36%   83.24%   -0.13%     
===========================================
  Files          161      161              
  Lines        13247    13396     +149     
===========================================
+ Hits         11043    11151     +108     
- Misses        2204     2245      +41     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

test/base/test_lazy_result.py Outdated Show resolved Hide resolved
test/base/test_lazy_result.py Show resolved Hide resolved
@dweindl
Copy link
Member

dweindl commented Jun 25, 2024

* [ ]  Do we want a casing limit, if yes how would that look like?

Not sure what you mean.

* [ ]  Should the lazy result actually be a subclass of the optimizer result?

I'd say so.

Something to be discussed: Should changes to the result just affect the OptimizerResult instance or should they be written to file?

* [ ]  Connected with the above, when printing the lazy result, all attributes are shown as `None`

Why aren't they loaded on demand then?

@PaulJonasJost
Copy link
Collaborator Author

Not sure what you mean.

Sorry meant caching. If we load very big hessians, we might want to throw them out later again? 🤔

Something to be discussed: Should changes to the result just affect the OptimizerResult instance or should they be written to file?

That's an interesting thought. I would say per default no, as it would not be a result object created by optimization but changed thereafter. But a function would probably be good to propagate changes to the file?

Why aren't they loaded on demand then?

Not entirely sure, it might be because in the OptimizerResult they are attributes while here they are properties? And so since LazyResult is a subclass, it will get initiated with those attributes as well 🤔 When calling optimizer_result.id it gives back the appropriate value, just not in the __repr__/print

@dilpath
Copy link
Member

dilpath commented Jun 25, 2024

I have been using hdfdict [1] with pydantic objects, no issues so far in my basic use cases (including numpy arrays), which has lazy loading on by default. Hopefully the lazy loading persists when a pydantic data object is reconstructed from HDF5 via hdfdict, but I haven't tested this yet.

So one alternative to this PR would be to refactor our results to be pydantic classes. I guess this might anyway occur when PEtab Result is supported in pyPESTO.

[1] https://github.com/SiggiGue/hdfdict/blob/master/hdfdict/hdfdict.py

@PaulJonasJost PaulJonasJost marked this pull request as ready for review July 3, 2024 08:52
@PaulJonasJost
Copy link
Collaborator Author

@dilpath as we talked about, I think it makes sense to merge it in and when petabResult is out, we might anyways make it obsolete? In that case i think this as an intermediate step should be fine.

@dilpath
Copy link
Member

dilpath commented Jul 25, 2024

Yes, fine for me 👍

@PaulJonasJost PaulJonasJost merged commit 71f7011 into develop Jul 29, 2024
18 checks passed
@PaulJonasJost PaulJonasJost deleted the lazy_optimizer_result branch July 29, 2024 13:38
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.

4 participants