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

Changes ConstraintCollocator attributes to read only properties. Replaces PR #315 #337

Merged
merged 7 commits into from
Feb 9, 2025

Conversation

Peter230655
Copy link
Contributor

Read only attributes:
Corrected the mistakes of previous ignorance: leading underscore only when the attribute is set.
Removed the printing in the test.

@moorepants moorepants changed the title corrected the bad mistakes. Removed pritning. Replaces PR #315 Changes ConstraintCollocator attributes to read only properties. Replaces PR #315 Feb 8, 2025
@moorepants moorepants mentioned this pull request Feb 8, 2025
import sympy.physics.mechanics as mech
from opty.direct_collocation import Problem, ConstraintCollocator

import pytest
Copy link
Member

Choose a reason for hiding this comment

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

All imports should be at the top of the file with the other imports.

Copy link
Member

Choose a reason for hiding this comment

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

I fixed it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I fixed it.

Thanks!
I always make such mistakes, when I copy the test function which I write to test it (and where I need these imports) to test_direct_collocation.py

The time_symbol is optional, but as we saw recently, it is a good idea to explicitly add it here and also in create_objective_function. Would it make sense to make it required? (might break old code, so maybe not a good idea?)

Copy link
Member

Choose a reason for hiding this comment

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

In hindsight, time symbol should be required, but it really only fails on CI. I'm not quite sure why but something with the hashing of python objects changes. Not sure...

Copy link
Member

Choose a reason for hiding this comment

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

(might break old code, so maybe not a good idea?)

Best we don't break old code.

@moorepants
Copy link
Member

There were two arange calls, deleted the second.

@moorepants moorepants merged commit 6f0dde6 into csu-hmc:master Feb 9, 2025
22 checks passed
@Peter230655 Peter230655 deleted the read-only-attributes-2 branch February 9, 2025 10:33
@Peter230655
Copy link
Contributor Author

:-)

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