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 if custom force is found #248

Closed

Conversation

mattwthompson
Copy link
Collaborator

This is a short-term band-aid for #247 - implementing them will take more than a trivial amount of time so we should at least error out instead of silently ignoring entire forces in user's systems.

@ijpulidos ijpulidos added this to the 0.11.3 milestone Jul 11, 2023
@ijpulidos ijpulidos self-requested a review July 11, 2023 14:47
@mikemhenry
Copy link
Collaborator

@mattwthompson @ijpulidos I closed a bunch of PRs that I think #290 took care of, is this one still needed?

Comment on lines 1046 to 1048
raise Exception(f"Two instances of force {force_name} appear in System")
if force_name not in supported_forces:
raise Exception(f"Custom forces not supported. Found force of type {force_name}.")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I should probably add I don't think this is the right exception to raise, I'm just not sure which it should be

Copy link
Collaborator

Choose a reason for hiding this comment

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

Eh, this package doesn't define many exceptions, but I made a new one

@mikemhenry
Copy link
Collaborator

Not sure why this PR isn't running, @mattwthompson can you remake it when you have the chance?
image

@mattwthompson
Copy link
Collaborator Author

I'm pretty sure #247 is unaddressed, it's not directly related to the other PRs we've had opened.

I'm not sure why I opened this on my fork, moving it to upstream now

@mikemhenry
Copy link
Collaborator

Ah and yes it was a fork thing and not a permissions thing, oops

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.

3 participants