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

Error messages #96

Merged
merged 9 commits into from
Dec 13, 2023
Merged

Error messages #96

merged 9 commits into from
Dec 13, 2023

Conversation

lnrekerle
Copy link
Collaborator

@lnrekerle lnrekerle commented Oct 27, 2023

#83 #52
This PR will update error messages and create logging files for Warnings/Info that will be easier for users to understand.

edit:
Also will close #108

@lnrekerle lnrekerle added the enhancement New feature or request label Oct 27, 2023
@lnrekerle lnrekerle self-assigned this Oct 27, 2023
This was linked to issues Oct 27, 2023
@lnrekerle lnrekerle requested a review from ielis November 30, 2023 19:38
@lnrekerle lnrekerle marked this pull request as ready for review November 30, 2023 19:38
@lnrekerle
Copy link
Collaborator Author

I think this is good for now, we will obviously want to add more errors as we continue.

src/genophenocorr/analysis/_commie.py Outdated Show resolved Hide resolved
src/genophenocorr/analysis/_commie.py Outdated Show resolved Hide resolved
src/genophenocorr/analysis/_config.py Outdated Show resolved Hide resolved
src/genophenocorr/analysis/_config.py Outdated Show resolved Hide resolved
src/genophenocorr/analysis/_config.py Show resolved Hide resolved
src/genophenocorr/preprocessing/_phenotype.py Outdated Show resolved Hide resolved
src/genophenocorr/preprocessing/_uniprot.py Outdated Show resolved Hide resolved
src/genophenocorr/preprocessing/_vep.py Outdated Show resolved Hide resolved
src/genophenocorr/preprocessing/_vep.py Outdated Show resolved Hide resolved
src/genophenocorr/preprocessing/_vep.py Outdated Show resolved Hide resolved
Copy link
Member

@ielis ielis left a comment

Choose a reason for hiding this comment

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

Hi @lnrekerle thanks a lot for the updates.

On the lower level, I think the changes look good except for that one thing in the builder that I mention below. Please consider changing that, because I think this is a bug that can deceive the users and cause confusion.

On the higher level, I would like to see how the logging looks like in an example analysis. Therefore, let's please wait with merging until you're finished with #46. Then we can see if the logging looks reasonable.

Thank you!

src/genophenocorr/analysis/_config.py Outdated Show resolved Hide resolved
src/genophenocorr/preprocessing/_phenotype.py Show resolved Hide resolved
@lnrekerle lnrekerle requested a review from ielis December 12, 2023 16:35
Copy link
Member

@ielis ielis left a comment

Choose a reason for hiding this comment

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

Hi @lnrekerle ,

I have some "cosmetic" suggestions. Please address them, if you wish so, and then feel free to merge.

Thanks!

src/genophenocorr/preprocessing/_vep.py Outdated Show resolved Hide resolved
src/genophenocorr/preprocessing/_vep.py Outdated Show resolved Hide resolved
src/genophenocorr/preprocessing/_vep.py Outdated Show resolved Hide resolved
@lnrekerle lnrekerle merged commit 8cd56b9 into develop Dec 13, 2023
4 checks passed
@lnrekerle lnrekerle deleted the Error-messages branch December 13, 2023 17:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
2 participants