Skip to content

Conversation

@Ig-dolci
Copy link
Contributor

No description provided.

Copy link
Contributor

@jrmaddison jrmaddison left a comment

Choose a reason for hiding this comment

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

Is there a use case for storing the restart dependencies with a SingleDiskStorageSchedule? This does let you replay the forward, but is there a reason to do that?

"""

def __init__(self, move_data=False):
def __init__(self, move_data=False, store_only_adj_deps=False):
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
def __init__(self, move_data=False, store_only_adj_deps=False):
def __init__(self, move_data=False, *, store_only_adj_deps=True):

Default False would be an API break, changing existing behaviour. New arguments should be keyword only.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. I just updated it.

@Ig-dolci
Copy link
Contributor Author

Ig-dolci commented Mar 20, 2025

Is there a use case for storing the restart dependencies with a SingleDiskStorageSchedule? This does let you replay the forward, but is there a reason to do that?

It is nothing related to SingleDiskStorageSchedule itself. The goal here is to make SingleDiskStorageSchedule more flexible for use cases like pyadjoint, where an adjoint dependency data set cannot always be correctly constructed during the taping process. See this related pyadjoint issue

@jrmaddison
Copy link
Contributor

The goal here is to make SingleDiskStorageSchedule more flexible for use cases like pyadjoint, where an adjoint dependency data set cannot always be correctly constructed during the taping process. See this related pyadjoint issue

Thanks, I see. There's nothing inherently wrong with adding a store_ics argument here if that's helpful, although it might fix the pyadjoint bug for the wrong reason. I'll comment there.

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.

3 participants