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

New URLConfig protocols - list-to-array and list-to-dict #1104

Merged
merged 10 commits into from
Oct 21, 2022

Conversation

LuisSanchez25
Copy link
Contributor

…gurations

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?

This code enables the user to change the format of the output of the url-protocol, some corrections require dictionaries while others require arrays. In Xedocs all of the data is stored in lists therefore this transformation stage is necessary to enable us to change the format of the output to a dictionary or an array.

Can you briefly describe how it works?

It works by taking the output of the URLConfig and depending on the added field changing the output from a list to either a dictionary with keys or an array.

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

This code reprocesses some XENONnT data using a correction stored in xedocs

import cutax
import xedocs as xd

run_id = '047493'
st_xd = cutax.contexts.xenonnt_online()
st_xd.set_config({"rel_extraction_eff":"objects-to-dict://xedocs://"
"rel_extraction_effs"
"?version=v!&context=staging_db&run_id=047122&partition=['ab','cd']&key_attr=partition&value_attr=value"})

kr_ei_xd = st_xd.get_array(run_id, 'event_info')

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.

@LuisSanchez25 LuisSanchez25 requested review from jmosbacher and removed request for jmosbacher October 18, 2022 19:48
@jmosbacher
Copy link
Contributor

jmosbacher commented Oct 18, 2022

@LuisSanchez25 thanks for this PR, I think its important to mention that these protocols can be used independently from xedocs and describe them in a generic manner in the comments and naming within the code.
e.g. i would replace docs with objects and maybe even generalize the objects-to-dicts protocol to handle lists of dicts as well as lists of python objects.

@jmosbacher jmosbacher changed the title Adding new protocols to change the format of the values from URLConfi… New URLConfig protocols - list-to-array and list-to-dict Oct 18, 2022
@jmosbacher
Copy link
Contributor

I made a few changes including comments. All that is missing are tests.

@JoranAngevaare
Copy link
Contributor

Tests also fail because of:

On the XENONnT fork we test with database access, on private forks there is no database access for security considerations.

I'll change the testing to fix this

@JoranAngevaare
Copy link
Contributor

github closed this PR by accident but the failing tests should be resolved in #1105

@jmosbacher
Copy link
Contributor

Thanks @JoranAngevaare !
@LuisSanchez25 just add tests and then you can mark this ready for review

@jmosbacher jmosbacher marked this pull request as ready for review October 20, 2022 19:39
Copy link
Contributor

@jmosbacher jmosbacher left a comment

Choose a reason for hiding this comment

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

I have made the changes I requested already.

@jmosbacher
Copy link
Contributor

@JoranAngevaare any comments before we merge?

Copy link
Contributor

@JoranAngevaare JoranAngevaare left a comment

Choose a reason for hiding this comment

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

Looks good. Only smaller suggestions, if you like any of them, you can add them but this state is already good. Thanks!

straxen/url_config.py Outdated Show resolved Hide resolved
straxen/url_config.py Outdated Show resolved Hide resolved
tests/test_url_config.py Outdated Show resolved Hide resolved
jmosbacher and others added 2 commits October 21, 2022 10:06
Co-authored-by: Joran R. Angevaare <joranangevaare@gmail.com>
Copy link
Contributor

@JoranAngevaare JoranAngevaare left a comment

Choose a reason for hiding this comment

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

Thanks! Looks good!

@jmosbacher jmosbacher merged commit 49b2657 into XENONnT:master Oct 21, 2022
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.

3 participants