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

update memory calculator #5244

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

Conversation

BrianMarre
Copy link
Member

@BrianMarre BrianMarre commented Dec 19, 2024

This PR,

  • refactors the memory calculator:
    • type validation using pydantic
    • function signature checks using typeguard
    • remove explicit passing of extents by dimension
    • changes particle attribute specification to be by list of names
  • refactors the use example
    • update to the new interfaces
    • generalize the implementation
    • switches to per gpu output instead of per row output
  • adds the memory calculation for atomic physics super cell fields and atomic physics particle attributes

@BrianMarre BrianMarre force-pushed the topic-updateMemoryCalculatorWithAtomicPhysis branch from 83ab2f0 to 702b222 Compare December 19, 2024 19:13
Copy link
Member

@pordyna pordyna left a comment

Choose a reason for hiding this comment

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

Nice! Looks mostly good to me, though I made some mostly just pedantic comments.

row_division = np.full(global_gpu_extent[dim], int(global_cell_extent[dim] / global_gpu_extent[dim]), dtype=np.int_)
global_domain_division.append(row_division)

print(f"global domain division: {global_domain_division}")
Copy link
Member

Choose a reason for hiding this comment

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

For me, it is not clear what domain division is supposed to be. I think you should name it differently.

Copy link
Member Author

Choose a reason for hiding this comment

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

after an offline discussion I followed Pawel's advice and renamed it to grid_distribution to match more closely the PIConGPU terminology.


print(f"global domain division: {global_domain_division}")

# get cell extent of each GPU, <extent of the gpu domain>[simulation dimension, multi dimensional gpu index]
Copy link
Member

Choose a reason for hiding this comment

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

Not sure I get this notation

Copy link
Member Author

Choose a reason for hiding this comment

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

rephrased the comment to make it clearer.

docs/source/usage/workflows/memoryPerDevice.py Outdated Show resolved Hide resolved
raise ValueError("unsupported precision {precision}")

@staticmethod
def get_predefined_attribute_dict(simulation_dimension: int, precision: int) -> dict[str, int]:
Copy link
Member

Choose a reason for hiding this comment

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

Why is it a static method? simulation_dimension, precision are also instance attributes. So as long as you put in the class, I would make just use these values.

Copy link
Member Author

Choose a reason for hiding this comment

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

since these methods might be usable in other contexts I decided to make them independent of the current implementation.

Copy link
Member

Choose a reason for hiding this comment

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

@BrianMarre ok, but in that case I would just take it out of the class :)

lib/python/picongpu/extra/utils/memory_calculator.py Outdated Show resolved Hide resolved
lib/python/picongpu/extra/utils/memory_calculator.py Outdated Show resolved Hide resolved
lib/python/picongpu/extra/utils/memory_calculator.py Outdated Show resolved Hide resolved
lib/python/picongpu/extra/utils/memory_calculator.py Outdated Show resolved Hide resolved
lib/python/picongpu/extra/utils/memory_calculator.py Outdated Show resolved Hide resolved
lib/python/picongpu/extra/utils/memory_calculator.py Outdated Show resolved Hide resolved
@BrianMarre BrianMarre force-pushed the topic-updateMemoryCalculatorWithAtomicPhysis branch from 702b222 to 8593bdb Compare December 20, 2024 13:46
ci: no-compile
@BrianMarre BrianMarre force-pushed the topic-updateMemoryCalculatorWithAtomicPhysis branch from 8593bdb to 79ee6ef Compare December 20, 2024 14:01
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