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

SR1 offline simulation strax context #1253

Merged
merged 10 commits into from Sep 8, 2023
Merged

SR1 offline simulation strax context #1253

merged 10 commits into from Sep 8, 2023

Conversation

shenyangshi
Copy link
Contributor

Before you submit this PR: make sure to put all operations-related information in a wiki-note, a PR should be about code and is publicly accessible

What does the code in this PR do / what does it improve?

Create a xedocs based simulation context using the exact corrections as real data. Simplify the logics.

Can you briefly describe how it works?

  1. Create a plain vanilla strax context
  2. Overwrite raw_records with wfsim raw_records
  3. Overwrite xedocs url config with a specific run's of the user's choice
  4. Add simulation needed configs in the strax config

Can you give a minimal working example (or illustrate with a figure)?

Please include the following if applicable:

  • Update the docstring(s)
  • Update the documentation
  • Tests to check the (new) code is working as desired.
  • Does it solve one of the open issues on github?

Notes on testing

  • Until the automated tests pass, please mark the PR as a draft.
  • On the XENONnT fork we test with database access, on private forks there is no database access for security considerations.

All italic comments can be removed from this template.

straxen/contexts.py Show resolved Hide resolved
straxen/contexts.py Outdated Show resolved Hide resolved
straxen/contexts.py Outdated Show resolved Hide resolved
straxen/contexts.py Outdated Show resolved Hide resolved
straxen/contexts.py Outdated Show resolved Hide resolved
straxen/contexts.py Show resolved Hide resolved
straxen/contexts.py Show resolved Hide resolved
straxen/contexts.py Show resolved Hide resolved
straxen/contexts.py Show resolved Hide resolved
straxen/contexts.py Show resolved Hide resolved
@shenyangshi shenyangshi marked this pull request as ready for review September 3, 2023 03:55
@shenyangshi
Copy link
Contributor Author

Copy link
Contributor

@ramirezdiego ramirezdiego left a comment

Choose a reason for hiding this comment

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

Thanks, @shenyangshi, it looks good to me, with a couple of comments for the future:

  1. I wonder if, although ugly (as is already the case), we could have integrated this new context options in the existing structure of xenonnt_simulation, since anyway the final usable context goes into cutax,
  2. We need to study the simulations case scenarios and rethink the (effectively deprecated) options to jump from processing to simulation configs and back, i.e., the current
        cmt_run_id_sim=None,
        cmt_run_id_proc=None,
        cmt_version='global_ONLINE',
        fax_config='fax_config_nt_design.json',
        overwrite_from_fax_file_sim=False,
        overwrite_from_fax_file_proc=False,
        ...

Let's anyway not complicate our lifes for the moment :) I'll let @dachengx merge.

@shenyangshi
Copy link
Contributor Author

Hi @ramirezdiego, thanks! I indeed thought about rewriting the xenonnt_simulation, but I don't want to break existing code that uses the cmt options a lot.

About the extra kwargs, I also thought about including them in the new context, but the namings are complex and even I cannot remember which is which. Maybe a better idea of the same comlexity is to modify st.config directly after setting up the context.

@dachengx
Copy link
Collaborator

dachengx commented Sep 7, 2023

Hi @ramirezdiego, thanks! I indeed thought about rewriting the xenonnt_simulation, but I don't want to break existing code that uses the cmt options a lot.

About the extra kwargs, I also thought about including them in the new context, but the namings are complex and even I cannot remember which is which. Maybe a better idea of the same comlexity is to modify st.config directly after setting up the context.

I agree that we should keep the old simulation context unchanged, because of the backward compatibility requirement.

@dachengx
Copy link
Collaborator

dachengx commented Sep 7, 2023

@shenyangshi Can this new context also be used on SR0?

@shenyangshi
Copy link
Contributor Author

Can this new context also be used on SR0?

Yes it can, if you give a SR0 run id and pick the SR0 fax_config.

@FaroutYLq
Copy link
Contributor

Hi @dachengx. Can we merge this and release v2.1.3 today?

@dachengx
Copy link
Collaborator

dachengx commented Sep 8, 2023

Would you add tests of the new context? Like:

return straxen.contexts.xenonnt_simulation(*args, **kwargs)
. @shenyangshi

@shenyangshi
Copy link
Contributor Author

@dachengx I think you can merge now.

@dachengx
Copy link
Collaborator

dachengx commented Sep 8, 2023

Hi @dachengx. Can we merge this and release v2.1.3 today?

Will merge if the test passes. Will release v2.1.3 if this is merged today.

@dachengx dachengx merged commit 3b8d13e into master Sep 8, 2023
8 of 9 checks passed
@dachengx dachengx deleted the xedocs_based_sim branch September 8, 2023 17:04
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.

None yet

4 participants