Conversation
Init Test Results 📝
|
Init Test Results 📝
|
Init Test Results 📝
|
There was a problem hiding this comment.
Thanks for adding this converter—this script looks like it takes SAR outputs (events.jsonl) and builds an RL-ready dataset. Since SAR itself is already part of the RL preprocessing pipeline, the filename soccer_rl_preprocessing feels a bit ambiguous.
Would you consider renaming it to reflect the SAR → RL-dataset step more explicitly?
Also, since this is a Python script (argparse + __main__), adding the .py extension would help IDEs/tools recognize it properly.
Init Test Results 📝
|
Init Test Results 📝
|
Init Test Results 📝
|
|
Thank you for your valuable feedback! |
Init Test Results 📝
|
|
@kenjiro-mk |
Init Test Results 📝
|
Init Test Results 📝
|
kenjiro-mk
left a comment
kenjiro-mk
left a comment
There was a problem hiding this comment.
Request:
I’d like preprocessing/sports/SAR_data/soccer/soccer_sar_to_rl_dataset.py to be runnable from SAR_data(...).preprocess_data() in the same way as the existing SAR pipeline. The switch should be done via preprocess_method.
Proposal (implementation):
- Extend the branching logic in
Soccer_SAR_data.preprocess_data()by adding a new case forpreprocess_method == "SAR2RL".SAR: keep the current SAR behavior as-is.SAR2RL: callsoccer_sar_to_rl_dataset.build_rl_datasets_from_sar_events(...)to generate the RL dataset.
Design points (review focus):
- Unified entry point
- This allows users to run it as
SAR_data(..., preprocess_method="SAR2RL").preprocess_data(...), which matches the “same as others” usage pattern and works cleanly after pip installation.
- Clarify input/output behavior
- For
SAR2RL, usepreprocessed_dir(existing argument / existing defaults) as the input root, and rely on the converter’s**/events.jsonlglobbing behavior. - Write outputs to a dedicated subdirectory such as
output_dir = <preprocessed_dir>/rl_dataset(or an equivalent deterministic subfolder) to avoid collisions with SAR outputs and prevent accidental overwrites.
- Minimize imports / avoid pulling heavy dependencies
soccer_sar_to_rl_dataset.pyis designed to be lightweight (only reading precomputedevents.jsonl).
Therefore, it’s preferable to import it locally inside theSAR2RLbranch, so running SAR2RL does not unnecessarily import the full SAR preprocessing stack.
- Tests and usage examples
- Adding a smoke test for
preprocess_method="SAR2RL"(a tiny dummyevents.jsonlis sufficient) would improve confidence. - Adding one usage example in the README showing
preprocess_method="SAR2RL"would help users avoid confusion.
|
@kenjiro-mk |
2 participants
It takes existing SAR output files (events.jsonl) and converts them into a clean, model-ready RL dataset that can be used by both DQN and QMIX.
What it does
Why this helps
Gives one shared, reproducible dataset format for fair DQN vs QMIX comparison.
Keeps RL tensorization logic separate from existing SAR processing code, so current pipelines are not affected.