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 Valdrada, the trigger simulation city #795

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
53 changes: 53 additions & 0 deletions invisible_cities/cities/valdrada_test.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
import os
import tables as tb

from . valdrada import valdrada
from .. core.testing_utils import assert_tables_equality


def test_valdrada_contains_all_tables(trigger_config):
conf, PATH_OUT = trigger_config
valdrada(**conf)
with tb.open_file(PATH_OUT) as h5out:
assert "Run" in h5out.root
assert "Run/events" in h5out.root
assert "Run/runInfo" in h5out.root
assert "Trigger" in h5out.root
assert "Trigger/events" in h5out.root
assert "Trigger/trigger" in h5out.root
assert "Trigger/DST" in h5out.root


def test_valdrada_exact_result_multipeak(ICDATADIR, trigger_config):
true_out = os.path.join(ICDATADIR, "test_trigger_fpga_multipeak.h5")
conf, PATH_OUT = trigger_config
valdrada(**conf)

tables = ("Trigger/events", "Trigger/trigger", "Trigger/DST",
"Run/events" , "Run/runInfo")

with tb.open_file(true_out) as true_output_file:
with tb.open_file(PATH_OUT) as output_file:
for table in tables:
assert hasattr(output_file.root, table)
got = getattr( output_file.root, table)
expected = getattr(true_output_file.root, table)
assert_tables_equality(got, expected)


def test_valdrada_exact_result(ICDATADIR, trigger_config):
Copy link
Collaborator

Choose a reason for hiding this comment

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

how about

@mark.parametrize("multipeak", (True, False))
def test_valdrada_exact_result(ICDATADIR, trigger_config, multipeak)
    ...
    if not multipeak:
        conf['trigger_config']['multipeak'] = None

to combine both tests? You would need to add true_out to the parametrize as well, but it's worth it.

true_out = os.path.join(ICDATADIR, "test_trigger_fpga.h5")
conf, PATH_OUT = trigger_config
conf['trigger_config']['multipeak'] = None
valdrada(**conf)

tables = ("Trigger/events", "Trigger/trigger", "Trigger/DST",
"Run/events" , "Run/runInfo")

with tb.open_file(true_out) as true_output_file:
with tb.open_file(PATH_OUT) as output_file:
for table in tables:
assert hasattr(output_file.root, table)
got = getattr( output_file.root, table)
expected = getattr(true_output_file.root, table)
assert_tables_equality(got, expected)
43 changes: 43 additions & 0 deletions invisible_cities/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,11 @@ def example_blr_wfs_filename(ICDATADIR):
return os.path.join(ICDATADIR, "blr_examples.h5")


@pytest.fixture(scope='session')
def example_blr_fpga_wfs_filename(ICDATADIR):
return os.path.join(ICDATADIR, "blr_fpga_examples.h5")


@pytest.fixture(scope = 'session',
params = ['electrons_40keV_z250_MCRD.h5'])
def electron_MCRD_file(request, ICDATADIR):
Expand Down Expand Up @@ -699,6 +704,44 @@ def deconvolution_config(ICDIR, ICDATADIR, PSFDIR, config_tmpdir):

return conf, PATH_OUT

<<<<<<< HEAD
=======
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unresolved conflict! Fixed in the last commit, but shouldn't be here anyway.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have no idea how to remove this from the history.


@pytest.fixture(scope='function') # Needs to be function as the config dict is modified when running
gonzaponte marked this conversation as resolved.
Show resolved Hide resolved
def trigger_config(ICDIR, ICDATADIR, config_tmpdir):
PATH_IN = os.path.join(ICDATADIR , "blr_fpga_examples.h5")
PATH_OUT = os.path.join(config_tmpdir, "trigger_city_out.h5")
nevt_req = 10
conf = dict(files_in = PATH_IN ,
file_out = PATH_OUT,
event_range = nevt_req,
compression = 'ZLIB4',
print_mod = 1000,
run_number = 8093,
trigger_config = dict(coincidence_window = 64
,discard_width = 40
,multipeak = dict(q_min = 100000
,time_min = 2 *units.mus
,time_after = 800*units.mus)),
channel_config = {0 : dict(q_min = 5000
,q_max = 50000
,time_min = 2 *units.mus
,time_max = 40*units.mus
,baseline_dev = 10
,amp_max = 1000
,pulse_valid_ext = 50*units.ns)
,2 : dict(q_min = 5000
,q_max = 50000
,time_min = 2 *units.mus
,time_max = 40*units.mus
,baseline_dev = 10
,amp_max = 1000
,pulse_valid_ext = 50*units.ns)})
Comment on lines +719 to +737
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add * adc and # time bins as discussed offline.


return conf, PATH_OUT


>>>>>>> f8fa2343... fixup! Add related tests
## To make very slow tests only run with specific option
def pytest_addoption(parser):
parser.addoption(
Expand Down
56 changes: 56 additions & 0 deletions invisible_cities/io/trigger_io_test.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
import os

import numpy as np
import tables as tb

from numpy.testing import assert_allclose
from hypothesis import given
from hypothesis.strategies import integers
from hypothesis.strategies import lists
from hypothesis.strategies import booleans
from hypothesis.strategies import composite

from ..core.testing_utils import assert_dataframes_equal
from ..io.dst_io import load_dst
from . trigger_io import trigger_dst_writer


@composite
def trigger_values(draw):
size = draw(integers(min_value=1, max_value=10))
elements = []
for _ in range(6):
elements.append(draw(lists(integers(min_value=0, max_value=np.iinfo(np.uint32).max),
min_size=size, max_size=size)))
for _ in range(4):
elements.append(draw(lists(booleans(), min_size=size, max_size=size)))
for _ in range(5):
elements.append(draw(lists(integers(min_value=np.iinfo(np.int32).min, max_value=np.iinfo(np.int32).max),
min_size=size, max_size=size)))
Copy link
Collaborator

Choose a reason for hiding this comment

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

do these numbers have specific meanings? Maybe it's better to give them names lke
a_meaningful_name = lists(integers(min_value=0, max_value=np.iinfo(np.uint32).max) and then draw(a_meaninful_name)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, I see that later these are assigned to each column in col_names. I think in terms of self-documentation it's better for the composite to draw each value individually, but this might be too slow (haven't checked), so at least you should leave a comment annotating the columns they correspond to.

I think I would also transpose the data drawing: instead of "for each column generate all rows" -> "for each row generate all columns". This would be more natural if we had a composite that generates a single row and then another one that generates a list of rows of a given size. If this doesn't affect performance, I would go for it.

return elements


@given(triggers=trigger_values())
def test_trigger_dst_writer(config_tmpdir, triggers):
output_file = os.path.join(config_tmpdir, "test_trigger_dst.h5")

trigger_save = [list(a) for a in zip(*triggers)]
with tb.open_file(output_file, 'w') as h5out:
write = trigger_dst_writer(h5out)
write(trigger_save)

trigger_dst = load_dst(output_file, group='Trigger', node='DST')

col_names = ["event" , "pmt" , "trigger_time" , "q"
,"width" , "height" , "valid_q" , "valid_w"
,"valid_h" , "valid_peak", "valid_all" , "baseline"
,"max_height", "n_coinc" , "closest_ttime", "closest_pmt"]

assert np.all([tname == cname for tname, cname in zip(trigger_dst.columns.values, col_names)])

# Check values stored, the if is needed because the valid_all condition is done in the writer, not on the input trigger
Copy link
Collaborator

Choose a reason for hiding this comment

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

The writer shouldn't manipulate the data. Why not add that column to the input instead?

for i, colname in enumerate(col_names):
if colname == 'valid_all':
assert_allclose(np.all(triggers[6:10], axis=0), trigger_dst.loc[:, colname])
elif i>10: assert_allclose(triggers[i-1] , trigger_dst.loc[:, colname])
else : assert_allclose(triggers[i ] , trigger_dst.loc[:, colname])
Loading