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

Saves pypesto and python version to the problem. #1382

Merged
merged 9 commits into from
May 26, 2024
Merged

Conversation

PaulJonasJost
Copy link
Collaborator

As the problem is generally saved alongside the result, it gives access to the version of pypesto and python that was used for optimization.

Would something like "metadata" of a result object make sense? Basically informative string representations of the engine, optimizer, problem/objective, alongside necessary versions?

@PaulJonasJost PaulJonasJost requested a review from dweindl May 2, 2024 11:55
@PaulJonasJost PaulJonasJost self-assigned this May 2, 2024
@codecov-commenter
Copy link

codecov-commenter commented May 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.66%. Comparing base (fefefd5) to head (77341e3).

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

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1382      +/-   ##
===========================================
- Coverage    83.96%   81.66%   -2.30%     
===========================================
  Files          160      160              
  Lines        13051    13055       +4     
===========================================
- Hits         10958    10662     -296     
- Misses        2093     2393     +300     

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

Copy link
Member

@dweindl dweindl left a comment

Choose a reason for hiding this comment

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

I think it makes sense to have that information somewhere.

Not sure whether it should be part of Problem or not. Is the assumption that one result file contains on results obtained from the same pypesto/python version? Then it would be sufficient to have it once per file. Otherwise, it should be saved with each Problem/OptimizerResult. The latter is probably safer, and makes it easier to read the version number to account for potential changes in the result structure.

Okay for me as a start.

pypesto/problem/base.py Outdated Show resolved Hide resolved
Copy link
Contributor

@m-philipps m-philipps left a comment

Choose a reason for hiding this comment

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

I agree that it's useful! are there cases where it's not possible to save the problem to hdf5?

Co-authored-by: Daniel Weindl <dweindl@users.noreply.github.com>
@PaulJonasJost
Copy link
Collaborator Author

I agree that it's useful! are there cases where it's not possible to save the problem to hdf5?

No there should not be any, but you could theoretically specify NOT to save it.

Is the assumption that one result file contains on results obtained from the same pypesto/python version?

Yes correct that is the assumption currently. I think long term having it in the OptimizerResult would perhaps be better with a set of other "metadata" collections. My list of things I would like to somewhat safe would be the following (happy for any additional ones)

  • Optimizer: version + settings
  • pypesto, python version
  • objective type (and if applicable version + basic settings)
  • engine type
  • Date of optimization

I think for many things it might be useful to alter the __str__ representation and just hand that over?

@dilpath
Copy link
Member

dilpath commented May 3, 2024

Could change class OptimizerResult(dict), class ProfileResult(dict), etc. to instead inherit from some class TaskResult(dict) that looks like

class TaskResult(dict):
    def __init__(self):
        self.set_common_metadata()
    
    def set_common_metadata(self):
        self.metadata = {
            "python_version": ...,
            "pypesto_version": ...,
        }

and then e.g.

class OptimizerResult(TaskResult):
    def __init__(self, ...):
        super().__init__()

    def set_task_metadata(self, optimizer: pypesto.optimize.Optimizer, engine: ...):
        self.metadata['optimizer'] = type(optimizer)
        self.metadata['engine'] = type(engine)
        ...

Then just need to make sure the TaskResult.metadata dict is saved in each result storage method, and call OptimizerResult.set_task_metadata(...) inside pypesto.optimize.minimize after creating the OptimizerResult.

@PaulJonasJost PaulJonasJost merged commit 4222672 into develop May 26, 2024
18 checks passed
@PaulJonasJost PaulJonasJost deleted the Some_metadata branch May 26, 2024 08:49
This was referenced May 27, 2024
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.

5 participants