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

Cy/issue 0070 #80

Merged
merged 6 commits into from
Jan 15, 2025
Merged

Cy/issue 0070 #80

merged 6 commits into from
Jan 15, 2025

Conversation

chyan26
Copy link
Collaborator

@chyan26 chyan26 commented Jan 14, 2025

I have created a master workflow as well as separated keywords, datasources and classifications. I want to merge it.

@chyan26
Copy link
Collaborator Author

chyan26 commented Jan 14, 2025

@hugobuddel Let me know if I can merge it. Thanks.

Copy link
Contributor

@hugobuddel hugobuddel left a comment

Choose a reason for hiding this comment

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

Thank you Chi-Hung!

Note that you should feel free to 'request' me as a reviewer by pressing the button at the top right of the PR page if you think a review specifically from me would be useful.

I highlighted two things that need to be changed at some point. I think the best moment is to do that now, but you can also merge this and do it later.

@@ -22,7 +22,7 @@ Including the following two line in your .bashrc
export METIS_SOFTPATH='<path_of_METIS_pipeline>'
export PYTHONPATH=$METIS_SOFTPATH
export SOF_DATA='<path_to_data>'
export PYCPL_RECIPE_DIR='<path_of_METIS_pipeline>/prototypes/recipes/'
export PYCPL_RECIPE_DIR='<path_of_METIS_pipeline>/metisp/pymetis/src/pymetis/recipes/'
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
export PYCPL_RECIPE_DIR='<path_of_METIS_pipeline>/metisp/pymetis/src/pymetis/recipes/'
export PYCPL_RECIPE_DIR='<path_of_METIS_pipeline>/metisp/pyrecipes/'

pyrecipes is the official path that should be used. So please make sure all recipes are actually imported there

from . import metis_keywords as metis_kwd

# Detector linearity calibration classification
detlin_class = classification_rule("DETLIN_2RG_RAW",
Copy link
Contributor

Choose a reason for hiding this comment

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

These classifications now all should have lm in their name, because they are not in an lm-specific file anymore. That is, how should we call the DETLIN_GEO_RAW now?

I think it would be easiest to just reuse the tag name, e.g. detlin_2rg_raw.

@chyan26 chyan26 merged commit 0afc2de into main Jan 15, 2025
2 checks passed
@hugobuddel
Copy link
Contributor

@chyan26 , I did expect you to at least acknowledge the comments that I did make. Like add a comment or so, but the suggestion on the README you could have simply accepted by pressing the "Accept Suggestion" button. You don't have to accept them, but I do think it is rude to just ignore my comments completely.

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.

2 participants