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

Fixes problem that NoneTypes are TypeErrors #65

Merged
merged 1 commit into from
Jul 22, 2024

Conversation

MilitaoLucas
Copy link
Contributor

The HBP on the API can return None if there is disorder in the coformers. None raises a TypeError in the MHBP. NoneTypes don't fall under RuntimeExceptions even though they are technically r This could be said to be a bad practice as some errors are due to problems that aren't individual to the coformer, but general system errors, like the R one. But, RuntimeException had the same problem, so I don't think in this case this is particularly bad.
I think that maybe in the future, the API could raise an DisorderException so users can treat exceptions less generally.

@pmbulit pmbulit self-assigned this Jul 19, 2024
@pmbulit
Copy link
Member

pmbulit commented Jul 19, 2024

What about doing this on l:269 instead? Not sure if the issue also happens with donors, but testing with FUTWOI37 this only happened for acceptors.

        try:
            ascores[a.label] = [round((coordination_scores.predictions_for_label(a.label, 'a')[1])[k], 3)
                            for k in coord_cols]
        except TypeError:
            raise RuntimeError

@MilitaoLucas
Copy link
Contributor Author

What about doing this on l:269 instead? Not sure if the issue also happens with donors, but testing with FUTWOI37 this only happened for acceptors.

        try:
            ascores[a.label] = [round((coordination_scores.predictions_for_label(a.label, 'a')[1])[k], 3)
                            for k in coord_cols]
        except TypeError:
            raise RuntimeError

I don't really know. But I think there is not much improvement on being more specific at this point. A RuntimeException already is very general, so being more general is not that bad IMO. I also tried doing this for acceptors and donors on propensity calc but thought it would be meaningless repetition. It could work, but I think it would be better to be more general and avoid any future errors that stop the entire run. It is extremely frustrating to let a library run overnight, being excited to do some experiments in the morning and being met with an exception exclusive to one pair API-coformer that stops everything.

Copy link
Member

@pmbulit pmbulit left a comment

Choose a reason for hiding this comment

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

Fair. The idea with the exceptions is that the run doesn't fail because one calculation failed so I've approved the changes

@pmbulit pmbulit merged commit 12688df into ccdc-opensource:main Jul 22, 2024
2 of 3 checks passed
@MilitaoLucas MilitaoLucas deleted the LucasMilitao2 branch August 22, 2024 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants