Conversation
Disable global version config check to avoid errors.
Add debug print statement in check_global_version_wrapper.
Comment out the code that overwrites the get_components method in strax.Context.
Added debug print statements for protocol evaluation and method return.
|
@WenzDaniel @dachengx I think if you would like to have a look at the changes most things make sense to me, and I see no issues or conflicts with the default pipeline. Just adding Vanilla/Online related stuff (plus scripts changes that are just DAQ stuff). Only thing still to fix is this merged field I removed for now. |
WenzDaniel
left a comment
WenzDaniel
left a comment
There was a problem hiding this comment.
Thanks Carlo I think it looks good. I have not checked if you copied the code correctly, but I assume you did. Neither did I test process some data, but here I assume you did as well. I cannot really judge the ajax and bootstrax changes though. Below you can find a few smaller questions from my side. I think generally the base is good to iterate from here.
| ("tight_coincidence", np.int16, "channel within tight range of mean"), | ||
| ("n_saturated_channels", np.int16, "total number of saturated channels"), | ||
| ("merged", bool, "is merged from peaklets"), | ||
| # ("merged", bool, "is merged from peaklets"), |
There was a problem hiding this comment.
Can you elaborate what the difficulty is? Is it because it is only in S2 and not S1?
There was a problem hiding this comment.
Simply I didn't have the time to manage to make it work with the MergedS2sVanilla. Just some issues with dtypes and stuff, and I didnt know how to assign its value. It should not be easy to add again. Maybe @dachengx do you want to give it a quick try?
| if isinstance(run_doc, list) and isinstance(run_doc[0], dict): | ||
| # Extract the dictionary | ||
| doc = run_doc[0] | ||
| doc = run_doc[-1] |
There was a problem hiding this comment.
There can be more than one document?
There was a problem hiding this comment.
The code in this plugin is actually misleading, what is called run_doc in the code is actually the comment of the rundoc (see the URLConfig, it calls comments on run_doc). So because sometimes the comment is wrong, you can updated it from the DAQ website, and this way we take the last one. So you can fix a comment for a run that would not work.
| @@ -54,7 +54,7 @@ def infer_dtype(self): | |||
| np.int16, | |||
| ), | |||
| (("Classification of the peak(let)", "type"), np.int8), | |||
There was a problem hiding this comment.
Even if it does not work at event basics we can still leave it here is not it?
There was a problem hiding this comment.
you mean the merged field? if we dont propagate it better to remove it..
| depends_on: Union[Tuple[str, ...], str] = ( | ||
| "peaklets", | ||
| "enhanced_peaklet_classification", | ||
| "merged_s2s", | ||
| ) | ||
|
|
||
| def infer_dtype(self): | ||
| # In case enhanced_peaklet_classification has more fields than peaklets, | ||
| # we need to merge them | ||
| peaklets_dtype = self.deps["peaklets"].dtype_for("peaklets") | ||
| peaklet_classification_dtype = self.deps["enhanced_peaklet_classification"].dtype_for( | ||
| "enhanced_peaklet_classification" | ||
| ) | ||
| merged_dtype = strax.merged_dtype((peaklets_dtype, peaklet_classification_dtype)) | ||
| # Numba is very picky about alignment for structured dtypes. Ensure an aligned dtype | ||
| # so assignments inside strax.replace_merged don't see "unaligned array(...)". | ||
| return np.dtype(merged_dtype, align=True) | ||
|
|
There was a problem hiding this comment.
Is this a AI addition? Can you elaborate?
There was a problem hiding this comment.
its from the peaks vanilla plugin... but I had to change the name of the deps.
this part:
#numba is very picky about alignment for structured dtypes. Ensure an aligned dtype
# so assignments inside strax.replace_merged don't see "unaligned array(...)".
return np.dtype(merged_dtype, align=True)
I dont remember if the unaligned array was a temporary error I had or if still needed..
| # # strax.replace_merged is numba-jitted and requires the two inputs to have the | ||
| # # exact same (and aligned) structured dtype. Some recent dtype-merging paths | ||
| # # produce unaligned dtypes, which triggers TypingError in numba. | ||
| # common_dtype = self.dtype | ||
|
|
||
| # _peaklets = np.zeros(len(peaklets), dtype=common_dtype) | ||
| # strax.copy_to_buffer(peaklets, _peaklets, f"_cast_{self.provides[0]}_peaklets") | ||
|
|
||
| # _merged_s2s = np.zeros(len(merged_s2s), dtype=common_dtype) | ||
| # strax.copy_to_buffer(merged_s2s, _merged_s2s, f"_cast_{self.provides[0]}_merged_s2s") | ||
|
|
||
| # _peaks = self.replace_merged(_peaklets, _merged_s2s, merge_s0=self.merge_s0) |
There was a problem hiding this comment.
Can this go? I assume that the copying is not needed anymore because of this unaligned/aligned thing. I would be still interested to understand what the problem was.
There was a problem hiding this comment.
yes this can go. Dont remember about the unaligned issue..
There was a problem hiding this comment.
Numba failing inside strax.replace_merged because it’s being fed an unaligned structured array dtype:
• Key clue: setitem(unaligned array(Record(...)), ...)
• Numba’s jitted _replace_merged does assignments like result[result_i] = merge[window_i], and it refuses when the structured dtype is unaligned (or when orig and merge are not exactly the same aligned dtype).
this was it
| straxen.MergedS2s.save_when = SaveWhen.TARGET | ||
| straxen.PeakletClassificationVanilla.save_when = SaveWhen.TARGET | ||
| straxen.PeakletPositionsCNF.save_when = SaveWhen.TARGET |
There was a problem hiding this comment.
Is it not possible to adjust the plugins themselves? Or does this collide with OSG reprocessing?
There was a problem hiding this comment.
Yeah, I am not sure why they were set like this, maybe needed for reprocessing?
This needs to be coordinated well with DAQ, Transfers, Reprocessing..
straxen/contexts.py
Outdated
| if _from_cutax and xedocs_version != "global_ONLINE": | ||
| context_options = {**straxen.contexts.common_opts, **kwargs} | ||
| else: | ||
| context_options = {**straxen.contexts.common_opts_vanilla, **kwargs} | ||
|
|
||
| st = straxen.contexts.xenonnt(**context_options) |
There was a problem hiding this comment.
This implies that we will never move back to the other plugins for online processing? I think it make sense, but was this discussed with a wider audience? However, if this is the case we also should not make this if/else statement for the "online" context, but always register the vanilla plugins.
There was a problem hiding this comment.
mmm "always" means in this straxen version, if this will change it would be anyway with a new straxen version where we can tune again the online context.
at the moment, i think its good to keep online from straxen using vanilla, and leave the option to use the common_opts if specifying online from cutax? idk.. this we can decide
|
Thank you @WenzDaniel for the careful review. |
Merging Option A implementation - matches non-vanilla architecture
for more information, see https://pre-commit.ci
The merged field assignment in PeakletClassificationVanilla.compute() was breaking PeakletClassificationSOM which inherits from it. Problem: - PeakletClassificationSOM inherits from PeakletClassificationVanilla - SOM plugin uses its own dtype (without merged field) - But calls parent's compute() which tried to set merged=False - Result: ValueError: no field of name merged (92 tests failed) Solution: - Make the assignment conditional: only set if field exists in dtype - This allows vanilla plugins to set it, SOM plugins to skip it safely This fixes all test failures in the standard xenonnt context while maintaining the merged field functionality in xenonnt_online context.
Also make merged field assignments conditional in: - merged_s2s_vanilla.py (line 159) - peaks_vanilla.py (line 80) These were still setting merged unconditionally, causing failures when SOM plugins inherit from vanilla but don't have merged in dtype. This completes the fix for all 92+ test failures.
* Add CI testing for xenonnt_online context This adds a new pytest_online job that runs the full test suite using the xenonnt_online context (vanilla plugins) instead of the default xenonnt context (SOM plugins). Changes: - tests/plugins/_core.py: Read STRAXEN_TEST_CONTEXT env var to select context - .github/workflows/pytest.yml: Add pytest_online to test matrix This will help catch vanilla plugin bugs (like the merged field issue) before merging to production. The online context is what DAQ actually uses, so direct testing is important. Test matrix after this change: - pytest_py3.10: xenonnt context - pytest_py3.11: xenonnt context - pytest_no_database_py3.10: xenonnt context - pytest_online_py3.10: xenonnt_online context (NEW) - coveralls_py3.10: xenonnt context * Add pytest-instafail for immediate test failure reporting Adds --instafail -vv --tb=short flags to all pytest runs to show failures immediately as they happen instead of at the end. This makes CI debugging much faster - you can see what's failing without waiting for the entire test suite to finish. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Exclude MLP plugins from xenonnt_online tests MLP position reconstruction plugins require model files that are not available in CI when using ONLINE corrections. Skip these plugins for xenonnt_online context tests to avoid download failures. The vanilla plugin logic is still tested with other plugins. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Fix: Set MLP models to None instead of excluding plugins Setting tf_model_mlp=None allows MLP plugins to run (returning NaN) without downloading model files. This preserves the dependency chain while avoiding model download issues in xenonnt_online CI tests. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Refactor: Test vanilla plugins using xenonnt context Instead of using xenonnt_online (which requires ONLINE corrections), test vanilla plugins by adding a _vanilla parameter to xenonnt() context. Changes: - Add _vanilla parameter to xenonnt() to use common_opts_vanilla - Add use_vanilla parameter to nt_test_context() - Rename pytest_online → pytest_vanilla in CI - Use STRAXEN_USE_VANILLA env var instead of STRAXEN_TEST_CONTEXT This avoids ONLINE corrections and xedocs model download issues while still testing vanilla plugin logic. * Use _vanilla flag in xenonnt_online context Make xenonnt_online use the _vanilla parameter when calling xenonnt() instead of manually merging common_opts. This simplifies the logic and ensures consistency. * Clean up redundant if/elif logic * Fix data_top support in vanilla and exclude SE score tests Changes: - Fix merged_s2s_vanilla.py: use store_data_top=True instead of digitize_top=True - Exclude SE score plugins from vanilla tests (advanced feature not supported) - SE score tests will be skipped for vanilla context This should fix test_sum_wf while properly excluding SE score tests. * Fix SE score test exclusion at test generation time The auto-generated tests are created at import time, before setUpClass runs. We need to check the STRAXEN_USE_VANILLA env var and exclude SE score plugins when generating the test list, not just when running setup. This properly prevents SE score tests from being generated for vanilla context. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Skip test_sum_wf for vanilla plugins The test_sum_wf checks the store_data_top feature, which requires data_top to be computed during peaklet creation. Vanilla plugins can't retroactively compute data_top values in merged_s2s. Reverted the store_data_top change in merged_s2s_vanilla as it doesn't actually fill data_top with values, just creates an empty field. Vanilla plugins work correctly without this advanced feature. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Re-enable test_sum_wf for vanilla Peaklets already has store_data_top=True by default for both SOM and vanilla. Since vanilla uses the same Peaklets plugin, data_top should be properly computed during peaklet creation. The merged_s2s_vanilla code that checks for data_top is just a fallback for edge cases. With store_data_top=True, vanilla should pass this test. * Fix indentation error in test_sum_wf Removed duplicate lines that were causing IndentationError * Enable store_data_top for vanilla contexts Root cause: merged_s2s_vanilla adds empty data_top field (zeros) as fallback when Peaklets doesn't compute it. But those zeros never get filled. Solution: Explicitly set store_data_top=True for vanilla contexts so Peaklets computes data_top during peaklet creation. This makes test_sum_wf pass by ensuring data_top contains actual top PMT waveform data instead of zeros. * Move store_data_top config to common_opts_vanilla Previous approach set config after context creation, which didn't affect the plugin initialization properly. Moving it to common_opts_vanilla ensures it's part of the context options from the start, so strax includes it in the lineage hash and properly initializes plugins with this config. * Fix config conflict in xenonnt() common_opts_vanilla now has config key, but xenonnt() was also passing config= to Context(). This caused 'multiple values for keyword argument config' error. Fixed by merging common_config with opts['config'] before passing to Context(). * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Skip test_sum_wf for vanilla (data_top issue) Despite setting store_data_top=True in common_opts_vanilla config, the data_top field still contains zeros instead of actual top PMT data. Root cause unclear - needs deeper investigation of why Peaklets plugin isn't properly computing data_top even with config set. Skipping this one test to unblock CI. Vanilla tests (319 other tests) should now pass. * Fix precommit issues 1. Add type annotation to exclude_plugins_vanilla to fix mypy error 2. Use raw string (r""") for RegexDispatcher docstring to fix SyntaxWarning about invalid escape sequences * Simplify vanilla config - remove store_data_top logic --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
|
Since the last review, I've added CI testing for vanilla plugins.. What changed:
Test results:
Overall the vanilla plugins work well including the new merged field (not sure if its implementation makes sense though) |
…n vanilla Root cause: merged_s2s_vanilla had two bugs: 1. Line 115: Used invalid parameter 'digitize_top=True' (should be 'store_data_top=True') 2. After merging peaks, properties were not recomputed, leaving area_fraction_top=0 Impact: All merged S2 peaks in vanilla had area_fraction_top=0, affecting: - S1/S2 discrimination - Position reconstruction - Energy reconstruction Fix: 1. Corrected parameter to store_data_top=True 2. Added strax.compute_properties() call after merge to recompute all peak properties 3. Removed test skip - test_sum_wf now passes for vanilla This fixes a critical data quality issue caught by the test.
|
Ok I figured out what was going on with the area_fraction_top test failing! The Problem: Root Cause:
Impact:
The Fix:
Good thing we added these tests, they caught a real bug! 🐛 |
|
Here the context comparison between "offline" and "online" contexts, meaning "default" vs "vanilla" plugins. 
|
|
I changed the way we start the mongo instance in the tests because it was giving: |
for more information, see https://pre-commit.ci
When store_data_start=False is set in Peaklets, the peaklets don't have
the data_start field. When MergedS2sVanilla adds the data_top field, it
was incorrectly checking if the OUTPUT (merged_s2s) should have data_start
instead of checking if the INPUT (peaklets) has it.
This caused a crash in strax.merge_peaks() when it tried to access
p['data_start'] on peaklets that don't have that field.
Fix: Check peaklets.dtype.names instead of self.dtype_for('merged_s2s').names
Strax's numba-compiled merge_peaks cannot handle dtypes where optional fields are missing - numba fails to compile any code that accesses p['data_start'] even if behind an 'if store_data_start:' guard. Workaround: Always include data_start field in buffer (stays zeros if not used). This adds ~800 bytes per merged S2, but avoids compilation errors. Field will be properly filled when peaklets have data_start, and stay zeros when they don't. TODO: Remove this workaround once strax properly supports optional fields via function overloading or separate code paths.
* what if we do only cnf on daq * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * fkn precommit * fix tests * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * fix tests * explicitly register normal peak positions * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * explicitly register normal peak positions --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
* what if we do only cnf on daq * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * fkn precommit * fix tests * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * fix tests * explicitly register normal peak positions * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * explicitly register normal peak positions * Add peak-only CNF position reconstruction for low-memory online processing - Create PeakPositionsCNFPeakOnly plugin that computes positions from peak area_per_channel - Does not depend on peaklet_positions, avoiding loading of all peaklet waveforms - Reduces RAM usage by ~27 GB (1.5M peaklets × 1KB × 15 workers) - Based on v2.2.7 approach (PeakPositionsBaseNT) - Add _low_memory_positions parameter to xenonnt() context - Enable by default in xenonnt_online() for DAQ Trade-offs: - Pro: ~27 GB RAM savings - Con: Slightly less accurate positions (no peaklet-level info) - Con: Less effective S2 merging (time-only, not time+position) This addresses the RAM increase from PR #1482 (peaklet-level positions) for online DAQ where RAM is constrained. Offline analysis can still use the full peaklet-level position reconstruction for better accuracy. * cleanup context * Fix PeakPositionsCNFPeakOnly to use proper CNF algorithm - Use JAX-based conditional normalizing flow (not TensorFlow MLP) - Include uncertainty contours and r/theta uncertainties - Properly format flow condition: normalized PMT pattern + log(area) - Match PeakletPositionsCNF compute logic but operate on peaks - Add helper methods: calculate_theta_diff, polygon_area, prediction_loop This correctly implements CNF position reconstruction at peak level without requiring peaklet waveforms. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * dont store data top and start * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * dont store data top and start --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
3 participants
This pull request introduces new "vanilla" versions of several key plugins and updates the context configuration to support them, primarily for the XENONnT experiment data processing. The changes include the addition of new plugins for merged S2 signals and peak position reconstruction, updates to context options to select between standard and vanilla processing chains, and various improvements and fixes in plugin dependencies and dtype handling.
New vanilla plugins and context options:
MergedS2sVanillaandMergedS2sHighEnergyVanillaplugins, along with their imports and registration, to provide alternative algorithms for merging S2 signals. [1] [2] [3] [4]PeakPositionsMLPVanillaandPeakPositionsCNFVanillaplugins for peak position reconstruction using MLP and CNF algorithms, respectively. [1] [2]straxen/contexts.pyto addcommon_opts_vanillaand logic inxenonnt_onlinefor selecting vanilla plugins, with additional debug print statements. [1] [2] [3]Plugin dependency and dtype handling improvements:
PeaksVanillato depend onpeaklet_classificationinstead ofenhanced_peaklet_classification, and improved dtype merging and alignment for compatibility with numba. [1] [2]PeaksSOMplugin to prevent numba errors.mergedfield in vanilla peak and event basics plugins to match the new vanilla processing chain. [1] [2] [3] (!!!!) This field we would actually like to add, but I did not manage to do it well!Bug fixes and minor changes:
is_the_led_onto use the last document in a list instead of the first.These changes collectively enable an alternative "vanilla" data processing path in the XENONnT analysis framework, with improved plugin modularity and maintainability.
References: