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 Pressure diag #772

Merged
merged 3 commits into from
Dec 20, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 36 additions & 0 deletions pyphare/pyphare/core/gridlayout.py
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,18 @@
"Vthx": "primal",
"Vthy": "primal",
"Vthz": "primal",
"Mxx": "primal",
"Mxy": "primal",
"Mxz": "primal",
"Myy": "primal",
"Myz": "primal",
"Mzz": "primal",
"Pxx": "primal",
"Pxy": "primal",
"Pxz": "primal",
"Pyy": "primal",
"Pyz": "primal",
"Pzz": "primal",
"tags": "dual",
},
"y": {
Expand All @@ -54,6 +66,18 @@
"Vthx": "primal",
"Vthy": "primal",
"Vthz": "primal",
"Mxx": "primal",
"Mxy": "primal",
"Mxz": "primal",
"Myy": "primal",
"Myz": "primal",
"Mzz": "primal",
"Pxx": "primal",
"Pxy": "primal",
"Pxz": "primal",
"Pyy": "primal",
"Pyz": "primal",
"Pzz": "primal",
"tags": "dual",
},
"z": {
Expand All @@ -77,6 +101,18 @@
"Vthx": "primal",
"Vthy": "primal",
"Vthz": "primal",
"Mxx": "primal",
"Mxy": "primal",
"Mxz": "primal",
"Myy": "primal",
"Myz": "primal",
"Mzz": "primal",
"Pxx": "primal",
"Pxy": "primal",
"Pxz": "primal",
"Pyy": "primal",
"Pyz": "primal",
"Pzz": "primal",
"tags": "dual",
},
}
Expand Down
2 changes: 1 addition & 1 deletion pyphare/pyphare/pharein/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -253,7 +253,7 @@ def as_paths(rb):

#### adding diagnostics
diag_path = "simulation/diagnostics/"
for diag in simulation.diagnostics:
for diag in list(simulation.diagnostics.values()):
type_path = diag_path + diag.type + "/"
name_path = type_path + diag.name
add_string(name_path + "/" + "type", diag.type)
Expand Down
88 changes: 68 additions & 20 deletions pyphare/pyphare/pharein/diagnostics.py
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,6 @@ def wrapper(diagnostics_object, name, **kwargs):

import numpy as np


# ------------------------------------------------------------------------------
def validate_timestamps(clazz, key, **kwargs):
sim = global_vars.sim
Expand Down Expand Up @@ -127,18 +126,30 @@ def __init__(self, name, **kwargs):
f"{self.__class__.__name__,}.flush_every cannot be negative"
)

if any(
[
self.quantity == diagnostic.quantity
for diagnostic in global_vars.sim.diagnostics
]
):
raise RuntimeError(
f"Error: Diagnostic ({kwargs['quantity']}) already registered"
)

self.__extent = None
global_vars.sim.add_diagnostics(self)

# if a diag already is registered we just contactenate the timestamps
addIt = True
registered_diags = global_vars.sim.diagnostics
for diagname, diag in registered_diags.items():
if self.quantity == diag.quantity:
print(
f"{diag.name} already registered {self.quantity}, merging timestamps"
)
my_times = self.write_timestamps
Copy link
Member

@PhilipDeegan PhilipDeegan Dec 14, 2023

Choose a reason for hiding this comment

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

my -> new?

you could remove it altogether and use it (self.write_timestamps) at np.concatenate(

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the proposed change is misleading. 'my' refers to 'self', 'new' refers to future self.
adding self.write_timestamps in the np.concatenates makes the line quite long and I find

' new_times = np.concatenate((my_times, existing_times))`

quite easy to read.

I don't think there is a perf issue as everything is just references anyway.

Copy link
Member

Choose a reason for hiding this comment

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

new IMO refers to additional, if there is existing

I have an aversion to my, implies personhood

existing_times = diag.write_timestamps
new_times = np.concatenate((my_times, existing_times))
new_times.sort()
mask = np.ones(len(new_times), dtype=bool)
mask[1:] = (
np.diff(new_times) > 1e-10
) # assumed smaller than any realistic dt
global_vars.sim.diagnostics[diagname].write_timestamps = new_times[mask]
addIt = False
break # there can be only one

if addIt:
global_vars.sim.add_diagnostics(self)
Comment on lines 126 to +152
Copy link

Choose a reason for hiding this comment

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

The logic for merging timestamps in the Diagnostics class could be simplified by using built-in numpy functions such as np.unique which can replace the manual creation of a mask and sorting.

-                new_times.sort()
-                mask = np.ones(len(new_times), dtype=bool)
-                mask[1:] = (
-                    np.diff(new_times) > 1e-10
-                )  # assumed smaller than any realistic dt
-                global_vars.sim.diagnostics[diagname].write_timestamps = new_times[mask]
+                unique_times = np.unique(new_times, axis=0)
+                global_vars.sim.diagnostics[diagname].write_timestamps = unique_times

Committable suggestion

IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
f"{self.__class__.__name__,}.flush_every cannot be negative"
)
if any(
[
self.quantity == diagnostic.quantity
for diagnostic in global_vars.sim.diagnostics
]
):
raise RuntimeError(
f"Error: Diagnostic ({kwargs['quantity']}) already registered"
)
self.__extent = None
global_vars.sim.add_diagnostics(self)
# if a diag already is registered we just contactenate the timestamps
addIt = True
registered_diags = global_vars.sim.diagnostics
for diagname, diag in registered_diags.items():
if self.quantity == diag.quantity:
print(
f"{diag.name} already registered {self.quantity}, merging timestamps"
)
my_times = self.write_timestamps
existing_times = diag.write_timestamps
new_times = np.concatenate((my_times, existing_times))
new_times.sort()
mask = np.ones(len(new_times), dtype=bool)
mask[1:] = (
np.diff(new_times) > 1e-10
) # assumed smaller than any realistic dt
global_vars.sim.diagnostics[diagname].write_timestamps = new_times[mask]
addIt = False
break # there can be only one
if addIt:
global_vars.sim.add_diagnostics(self)
f"{self.__class__.__name__,}.flush_every cannot be negative"
)
self.__extent = None
# if a diag already is registered we just contactenate the timestamps
addIt = True
registered_diags = global_vars.sim.diagnostics
for diagname, diag in registered_diags.items():
if self.quantity == diag.quantity:
print(
f"{diag.name} already registered {self.quantity}, merging timestamps"
)
my_times = self.write_timestamps
existing_times = diag.write_timestamps
new_times = np.concatenate((my_times, existing_times))
unique_times = np.unique(new_times, axis=0)
global_vars.sim.diagnostics[diagname].write_timestamps = unique_times
addIt = False
break # there can be only one
if addIt:
global_vars.sim.add_diagnostics(self)


def extent(self):
return self.__extent
Expand All @@ -162,6 +173,7 @@ def __init__(self, **kwargs):
)

def _setSubTypeAttributes(self, **kwargs):

if kwargs["quantity"] not in ElectromagDiagnostics.em_quantities:
error_msg = "Error: '{}' not a valid electromag diagnostics : " + ", ".join(
Comment on lines +176 to 178
Copy link

Choose a reason for hiding this comment

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

The error message in the ElectromagDiagnostics class could be more specific by stating that the provided quantity is not a valid electromag diagnostics.

-            error_msg = "Error: '{}' not a valid electromag diagnostics : " + ", ".join(
+            error_msg = "Error: '{}' is not a valid electromag diagnostics quantity. Valid quantities are: " + ", ".join(

Committable suggestion

IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
if kwargs["quantity"] not in ElectromagDiagnostics.em_quantities:
error_msg = "Error: '{}' not a valid electromag diagnostics : " + ", ".join(
if kwargs["quantity"] not in ElectromagDiagnostics.em_quantities:
error_msg = "Error: '{}' is not a valid electromag diagnostics quantity. Valid quantities are: " + ", ".join(

ElectromagDiagnostics.em_quantities
Comment on lines +176 to 179
Copy link

Choose a reason for hiding this comment

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

The error message in the ElectromagDiagnostics class could be more specific by stating that the provided quantity is not a valid electromag diagnostics quantity.

-            error_msg = "Error: '{}' not a valid electromag diagnostics : " + ", ".join(
+            error_msg = "Error: '{}' is not a valid electromag diagnostics quantity. Valid quantities are: " + ", ".join(

Committable suggestion

IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
if kwargs["quantity"] not in ElectromagDiagnostics.em_quantities:
error_msg = "Error: '{}' not a valid electromag diagnostics : " + ", ".join(
ElectromagDiagnostics.em_quantities
if kwargs["quantity"] not in ElectromagDiagnostics.em_quantities:
error_msg = "Error: '{}' is not a valid electromag diagnostics quantity. Valid quantities are: " + ", ".join(
ElectromagDiagnostics.em_quantities

Expand All @@ -188,14 +200,21 @@ def population_in_model(population):
return population in [p for p in global_vars.sim.model.populations]


class FluidDiagnostics(Diagnostics):
fluid_quantities = ["density", "flux", "bulkVelocity"]
class FluidDiagnostics_(Diagnostics):

fluid_quantities = [
"density",
"mass_density",
"flux",
"bulkVelocity",
"momentum_tensor",
]
type = "fluid"

def __init__(self, **kwargs):
super(FluidDiagnostics, self).__init__(
FluidDiagnostics.type
+ str(global_vars.sim.count_diagnostics(FluidDiagnostics.type)),
super(FluidDiagnostics_, self).__init__(
FluidDiagnostics_.type
+ str(global_vars.sim.count_diagnostics(FluidDiagnostics_.type)),
**kwargs,
)

Expand All @@ -206,15 +225,17 @@ def _setSubTypeAttributes(self, **kwargs):
elif "population_name" in kwargs:
self.population_name = kwargs["population_name"]

if kwargs["quantity"] not in FluidDiagnostics.fluid_quantities:
if kwargs["quantity"] not in FluidDiagnostics_.fluid_quantities:
error_msg = "Error: '{}' not a valid fluid diagnostics : " + ", ".join(
FluidDiagnostics.fluid_quantities
FluidDiagnostics_.fluid_quantities
Comment on lines +228 to +230
Copy link

Choose a reason for hiding this comment

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

The error handling in FluidDiagnostics_ for invalid quantities could be improved by providing a more informative error message.

-            error_msg = "Error: '{}' not a valid fluid diagnostics : " + ", ".join(
+            error_msg = "Error: '{}' is not a valid fluid diagnostics quantity. Valid quantities are: " + ", ".join(

Committable suggestion

IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
if kwargs["quantity"] not in FluidDiagnostics_.fluid_quantities:
error_msg = "Error: '{}' not a valid fluid diagnostics : " + ", ".join(
FluidDiagnostics.fluid_quantities
FluidDiagnostics_.fluid_quantities
if kwargs["quantity"] not in FluidDiagnostics_.fluid_quantities:
error_msg = "Error: '{}' is not a valid fluid diagnostics quantity. Valid quantities are: " + ", ".join(
FluidDiagnostics_.fluid_quantities

)
raise ValueError(error_msg.format(kwargs["quantity"]))
elif kwargs["quantity"] == "flux" and kwargs["population_name"] == "ions":
raise ValueError(
"'flux' is only available for specific populations, try 'bulkVelocity"
)
elif kwargs["quantity"] == "pressure_tensor":
raise ValueError("'pressure_tensor' is not available yet")
Comment on lines +228 to +238
Copy link

Choose a reason for hiding this comment

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

The error handling in FluidDiagnostics_ for invalid quantities could be improved by providing a more informative error message.

-            error_msg = "Error: '{}' not a valid fluid diagnostics : " + ", ".join(
+            error_msg = "Error: '{}' is not a valid fluid diagnostics quantity. Valid quantities are: " + ", ".join(

Committable suggestion

IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
if kwargs["quantity"] not in FluidDiagnostics_.fluid_quantities:
error_msg = "Error: '{}' not a valid fluid diagnostics : " + ", ".join(
FluidDiagnostics.fluid_quantities
FluidDiagnostics_.fluid_quantities
)
raise ValueError(error_msg.format(kwargs["quantity"]))
elif kwargs["quantity"] == "flux" and kwargs["population_name"] == "ions":
raise ValueError(
"'flux' is only available for specific populations, try 'bulkVelocity"
)
elif kwargs["quantity"] == "pressure_tensor":
raise ValueError("'pressure_tensor' is not available yet")
if kwargs["quantity"] not in FluidDiagnostics_.fluid_quantities:
error_msg = "Error: '{}' is not a valid fluid diagnostics quantity. Valid quantities are: " + ", ".join(
FluidDiagnostics_.fluid_quantities
)
raise ValueError(error_msg.format(kwargs["quantity"]))
elif kwargs["quantity"] == "flux" and kwargs["population_name"] == "ions":
raise ValueError(
"'flux' is only available for specific populations, try 'bulkVelocity"
)
elif kwargs["quantity"] == "pressure_tensor":
raise ValueError("'pressure_tensor' is not available yet")

else:
self.quantity = kwargs["quantity"]

Expand All @@ -232,7 +253,7 @@ def _setSubTypeAttributes(self, **kwargs):
def to_dict(self):
return {
"name": self.name,
"type": FluidDiagnostics.type,
"type": FluidDiagnostics_.type,
"quantity": self.quantity,
"write_timestamps": self.write_timestamps,
"compute_timestamps": self.compute_timestamps,
Expand All @@ -241,6 +262,25 @@ def to_dict(self):
}


def for_total_ions(**kwargs):
return "population_name" not in kwargs


class FluidDiagnostics:
def __init__(self, **kwargs):
if kwargs["quantity"] == "pressure_tensor":
if for_total_ions(**kwargs):
needed_quantities = ["mass_density", "bulkVelocity", "momentum_tensor"]
else:
needed_quantities = ["density", "flux", "momentum_tensor"]

for quantity in needed_quantities:
kwargs["quantity"] = quantity
FluidDiagnostics_(**kwargs)
else:
FluidDiagnostics_(**kwargs)


# ------------------------------------------------------------------------------


Expand All @@ -262,6 +302,12 @@ def _setSubTypeAttributes(self, **kwargs):
)
raise ValueError(error_msg.format(kwargs["quantity"]))

if kwargs["quantity"] not in ParticleDiagnostics.particle_quantities:
error_msg = "Error: '{}' not a valid particle diagnostics : " + ", ".join(
ParticleDiagnostics.particle_quantities
)
raise ValueError(error_msg.format(kwargs["quantity"]))
Comment on lines +305 to +309
Copy link

Choose a reason for hiding this comment

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

There is redundant code in the ParticleDiagnostics class where the quantity attribute is checked twice against particle_quantities.

-        if kwargs["quantity"] not in ParticleDiagnostics.particle_quantities:
-            error_msg = "Error: '{}' not a valid particle diagnostics : " + ", ".join(
-                ParticleDiagnostics.particle_quantities
-            )
-            raise ValueError(error_msg.format(kwargs["quantity"]))

Committable suggestion

IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
if kwargs["quantity"] not in ParticleDiagnostics.particle_quantities:
error_msg = "Error: '{}' not a valid particle diagnostics : " + ", ".join(
ParticleDiagnostics.particle_quantities
)
raise ValueError(error_msg.format(kwargs["quantity"]))


self.quantity = kwargs["quantity"]

self.space_box(**kwargs)
Expand All @@ -281,6 +327,7 @@ def _setSubTypeAttributes(self, **kwargs):
self.quantity = "/ions/pop/" + self.population_name + "/" + self.quantity

def space_box(self, **kwargs):

if "extent" not in kwargs and self.quantity == "space_box":
raise ValueError(
"Error: missing 'extent' parameter required by 'space_box' the ParticleDiagnostics type"
Expand All @@ -305,6 +352,7 @@ def to_dict(self):


class MetaDiagnostics(Diagnostics):

meta_quantities = ["tags"]
type = "info"

Expand Down
14 changes: 10 additions & 4 deletions pyphare/pyphare/pharein/simulation.py
Original file line number Diff line number Diff line change
Expand Up @@ -810,7 +810,7 @@ def __init__(self, **kwargs):

self.ndim = compute_dimension(self.cells)

self.diagnostics = []
self.diagnostics = {}
self.model = None
self.electrons = None

Expand Down Expand Up @@ -869,7 +869,7 @@ def __setstate__(self, state):
# ------------------------------------------------------------------------------

def add_diagnostics(self, diag):
if diag.name in [diagnostic.name for diagnostic in self.diagnostics]:
if diag.name in self.diagnostics:
raise ValueError(
"Error: diagnostics {} already registered".format(diag.name)
)
Expand All @@ -880,7 +880,7 @@ def add_diagnostics(self, diag):
):
raise RuntimeError("Error: invalid diagnostics spatial extent")

self.diagnostics.append(diag)
self.diagnostics[diag.name] = diag

# ------------------------------------------------------------------------------

Expand All @@ -893,7 +893,13 @@ def is_restartable_compared_to(self, sim):
# ------------------------------------------------------------------------------

def count_diagnostics(self, type_name):
return len([diag for diag in self.diagnostics if diag.type == type_name])
return len(
[
diag
for diagname, diag in self.diagnostics.items()
if diag.type == type_name
]
)

# ------------------------------------------------------------------------------

Expand Down
Loading
Loading