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.

Sign up for GitHub

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

Jump to bottom

Remove redundant pos recon #1449

Merged
merged 21 commits into from
Oct 17, 2024
Merged

Remove redundant pos recon #1449

merged 21 commits into from
Oct 17, 2024

Conversation

yuema137
Copy link
Collaborator

@yuema137 yuema137 commented Oct 15, 2024 •
edited
Loading

#1444
The only point that may need to be discussed is that the cnf based corrections are not available yet. So, I had to remove them from corrections_services for now. At some point we need to add this back

@yuema137 yuema137 marked this pull request as draft October 15, 2024 04:54
@yuema137 yuema137 mentioned this pull request Oct 15, 2024
Closed
@coveralls
Copy link

coveralls commented Oct 15, 2024 •
edited
Loading

Coverage Status

coverage: 90.235% (+0.02%) from 90.215%
when pulling 6a8fc8a on remove_redundant_pos_recon
into 2147ad1 on master.

dachengx
dachengx requested changes Oct 16, 2024
View reviewed changes
straxen/analyses/posrec_comparison.py
@@ -13,7 +13,7 @@ def load_corrected_positions(
alt_s1=False,
alt_s2=False,
cmt_version=None,
posrec_algos=("mlp", "gcn", "cnn"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why no "cnf" here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It compares the FDC map, and the cnf one is currently unavailable

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please make an issue about this.

straxen/corrections_services.py Outdated Show resolved Hide resolved
straxen/plugins/events/__init__.py Outdated Show resolved Hide resolved
straxen/plugins/peaks/__init__.py Outdated Show resolved Hide resolved
@juehang
Copy link
Contributor

juehang commented Oct 16, 2024

Looks good, I assume the tests failing is due to #1452 ? Also, is the plan to push this for now and add another PR after reprocessing and after the FDC map has been constructed?

@yuema137
Copy link
Collaborator Author

@juehang Yes see this issue: #1454
The analyses module is actually not super concerned, as there are no further dependencies. When the map is ready, we can add it back.

@dachengx dachengx marked this pull request as ready for review October 16, 2024 22:00
@dachengx
Copy link
Collaborator

@yuema137 add a TODO then?

dachengx
dachengx requested changes Oct 16, 2024
View reviewed changes
straxen/corrections_services.py Outdated Show resolved Hide resolved
straxen/plugins/events/__init__.py Outdated Show resolved Hide resolved
@dachengx dachengx merged commit 1f80838 into master Oct 17, 2024
8 checks passed
@dachengx dachengx deleted the remove_redundant_pos_recon branch October 17, 2024 00:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dachengx dachengx dachengx approved these changes

@juehang juehang Awaiting requested review from juehang

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@yuema137 @coveralls @juehang @dachengx