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

Remove CMT URLs #1235

Merged
merged 34 commits into from
Jan 23, 2025
Merged

Remove CMT URLs #1235

merged 34 commits into from
Jan 23, 2025

Conversation

jmosbacher
Copy link
Contributor

  • Replace all CMT URLs with xedoc URLs
  • unify "cmt_version" and "xedocs_version" arguments in the context functions, replace with single "global_version" argument which loads configs from xedocs.

straxen/plugins/afterpulses/afterpulse_processing.py Outdated Show resolved Hide resolved
straxen/plugins/defaults.py Outdated Show resolved Hide resolved
straxen/plugins/defaults.py Outdated Show resolved Hide resolved
straxen/plugins/events/corrected_areas.py Outdated Show resolved Hide resolved
straxen/plugins/events/corrected_areas.py Outdated Show resolved Hide resolved
straxen/plugins/peaks/peak_positions_gcn.py Outdated Show resolved Hide resolved
straxen/plugins/peaks/peak_positions_mlp.py Outdated Show resolved Hide resolved
straxen/plugins/records/records.py Outdated Show resolved Hide resolved
straxen/plugins/records_he/records_he.py Outdated Show resolved Hide resolved
straxen/plugins/records_nv/records_nv.py Outdated Show resolved Hide resolved
@GiovanniVolta
Copy link
Contributor

GiovanniVolta commented Nov 18, 2024

@LuisSanchez25 is this PR important for migrating from CMT to xedocs?

@LuisSanchez25
Copy link
Contributor

Depends what you mean by it. This PR mainly changes the default values is straxen, which are rarely used anyways, so it should not affect the processing of things, however, to transition to xedocs fully this is something that should be done eventually but its not immediately necessary.

The main issue I believe is that there are some tests that are designed around CMT that we need to rewrite to work with xedocs. I will ask @Ananthu-Ravindran if he thinks he could work on this.

@GiovanniVolta
Copy link
Contributor

Let me also remind you that @PeterGy offers his help for xedocs-related business. This is good way to start

@PeterGy
Copy link
Contributor

PeterGy commented Nov 19, 2024

It seems the logs for what tests fail have been deleted due to lack of retention (the tests were ran back in April). It would be useful to rerun them to see where and how they fail. Unfortunately, I do not seem to have permission to rerun any tests on this repo.

@LuisSanchez25
Copy link
Contributor

It seems the logs for what tests fail have been deleted due to lack of retention (the tests were ran back in April). It would be useful to rerun them to see where and how they fail. Unfortunately, I do not seem to have permission to rerun any tests on this repo.

One the conflicts are squashed we can make this into a PR instead of a draft PR, then the tests will run automatically

@dachengx dachengx mentioned this pull request Nov 20, 2024
@dachengx dachengx changed the title Remove cmt urls Remove CMT urls Dec 16, 2024
@dachengx dachengx changed the title Remove CMT urls Remove CMT URLs Dec 16, 2024
@GiovanniVolta
Copy link
Contributor

The URL config should match the global_ONLINE configuration. Right now it is not the case for many entries

@dachengx
Copy link
Collaborator

By running https://github.com/XENONnT/analysiscode/blob/17cd10bbb8d95ee54a45dd99f931a1ce1995e368/straxen_debug/bash_scripts/online_comparison.sh

The difference between this PR and https://github.com/XENONnT/corrections/blob/33c6004e256f42d3ee7c1cc8faf4bb438ba02991/XENONnT/global_versions/global_ONLINE.json is only

baseline_samples_nv:
---    OLD: cmt://baseline_samples_nv?version=ONLINE&run_id=plugin.run_id
---    NEW: 26
--- ONLINE: MISSING

tf_event_model_mlp:
---    OLD: tf://resource://cmt://mlp_model?version=ONLINE&run_id=plugin.run_id&fmt=abs_path
---    NEW: MISSING
--- ONLINE: MISSING

tf_event_model_s1_cnn:
---    OLD: tf://resource://xedocs://posrec_models?version=ONLINE&run_id=plugin.run_id&kind=s1_cnn&fmt=abs_path&attr=value
---    NEW: MISSING
--- ONLINE: MISSING
  1. baseline_samples_nv is not needed, see https://github.com/XENONnT/corrections/pull/336
  2. tf_event_model_mlp and tf_event_model_s1_cnn are not needed

@dachengx
Copy link
Collaborator

dachengx commented Jan 23, 2025

The test will not pass because some maps are not correct:

  1. gain_model_nv: https://xenonnt.slack.com/archives/C017UQVTUSE/p1737627605813549
  2. fdc_maps: https://xenonnt.slack.com/archives/C049JJZ2U3A/p1737627923037799

And I will merge this PR, because no more time should be wasted.

@dachengx dachengx marked this pull request as ready for review January 23, 2025 10:33
@dachengx dachengx merged commit 8fa804e into master Jan 23, 2025
4 of 8 checks passed
@dachengx dachengx deleted the remove_cmt_urls branch January 23, 2025 10:51
@GiovanniVolta
Copy link
Contributor

Dacheng, I don't agree with having merge this PR. I would like to ask you to roll it back and let me finish the review, please

@dachengx dachengx mentioned this pull request Jan 23, 2025
@GiovanniVolta
Copy link
Contributor

Although I would prefer to wait, I don't see a mistake in this PR

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.

7 participants