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

add a cache toggle #13

Open
wants to merge 4 commits into
base: parallel_count
Choose a base branch
from

Conversation

chrishavlin
Copy link

This is mostly for testing purposes, but maybe it'd be useful to include in the actual PR?

This PR adds a way to toggle the mask cache behavior for all the grids, the following would turn off/on cacheing:

ds = yt.load(...)
ds.index.set_grid_cache_mask(False) # Turn off
ds.index.set_grid_cache_mask(True) # Turn on

It includes an optional parameter, clear_all_data (default is True) to calls clear_all_data() if True.

@chrishavlin
Copy link
Author

@matthewturk I can fix that type checking error if you think this would be a useful functionality to expose, but if we want to keep this branch just for testing then I won't bother fixing it :)

@matthewturk
Copy link
Owner

I do think it is worthwhile.

@chrishavlin chrishavlin changed the title (maybe do not merge) add a cache toggle add a cache toggle Aug 29, 2023
@chrishavlin
Copy link
Author

removed the type hints i added to the new function cause it was triggering a type check for the whole class and I didn't want to get into fixing typing errors in this PR...

@chrishavlin
Copy link
Author

I think I'll add a simple test too...

@chrishavlin
Copy link
Author

glad i added a test -- clear_data() wasn't actually clearing the cached grid masks. So I adjusted the AMRGridPatch to reset the mask-related attributes.

@chrishavlin
Copy link
Author

@matthewturk just FYI, I'm not planning on modifying this anymore unless you've got comments.

Copy link
Owner

@matthewturk matthewturk left a comment

Choose a reason for hiding this comment

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

Looks great!

@@ -106,6 +106,27 @@ def clear_all_data(self):
g.clear_data()
self.io.queue.clear()

def set_grid_cache_mask(self, cache_grid_masks, clear_all_data=True):
Copy link
Owner

Choose a reason for hiding this comment

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

I'm slightly unsure about this being a top-level method, rather than having an underscore prefix. Then again, maybe it's good.

Copy link
Author

Choose a reason for hiding this comment

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

Ya, I had the same thought. I think an underscore might be better? It's probably not something 99.9% of people would use. But maybe someone might find themselves in a memory-limited situation and want to disable cacheing easily?

So ya, I was on the fence, but happy to go with an underscore instead.

Copy link
Owner

Choose a reason for hiding this comment

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

Let's do underscore, then I can merge and we'll PR upstream

Copy link
Author

Choose a reason for hiding this comment

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

done

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