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

Update on constraint module; Add generalized constraint class #56

Merged
merged 12 commits into from
Feb 12, 2022

Conversation

skim0119
Copy link
Collaborator

Dev Note

Related to #54 , the current constraint component does not pass the indices to the class methods. As a result, it is difficult to implement constraint or boundary conditions at the middle of the rod. I talked to @tp5uiuc, and this is definitely not intended behavior.

To fix the issue and improve future functionality, I'm exposing system(rod), position indices, and director indices to the constraining/BC class. This should make BC implementation more flexible.

In addition, I refactored @nmnaughton 's implementation on more general BC: FixedRodBC and FixedNodeBC (#54). I added an example script on how to use new constraint FixedConstraint. The PR also includes test codes.

How to use FixedConstraint

image

Update

  • ConstraintBase abstract class is added to provide template for all constraint classes.
    • People who inherited FreeRod should not get errors, but a deprecation warning should appear.
  • Properties for system, position_indices, and director_indices are added.
  • FreeRod and OneEndFixedRod are refactored to FreeBC and OneEndFixedBC. I made an alias and deprecation note so that people have time to notice the change and convert their code.
  • Boundary condition FixedConstraint with documentation.
  • Test code for FixedConstraint is added and tested.
  • Test code for the ConstraintBase class is added and tested.
  • Documentation is added for new classes.

resolve #55

@skim0119 skim0119 added enhancement New feature or request update Update the codebase labels Feb 10, 2022
@skim0119 skim0119 added this to the Version 0.2.2 milestone Feb 10, 2022
@skim0119 skim0119 self-assigned this Feb 10, 2022
@codecov-commenter
Copy link

codecov-commenter commented Feb 10, 2022

Codecov Report

Merging #56 (c629a78) into update-0.2.2 (1c008ba) will increase coverage by 0.43%.
The diff coverage is 96.63%.

Impacted file tree graph

@@               Coverage Diff                @@
##           update-0.2.2      #56      +/-   ##
================================================
+ Coverage         81.26%   81.70%   +0.43%     
================================================
  Files                40       40              
  Lines              2552     2645      +93     
  Branches            357      372      +15     
================================================
+ Hits               2074     2161      +87     
- Misses              453      458       +5     
- Partials             25       26       +1     
Impacted Files Coverage Δ
elastica/boundary_conditions.py 95.36% <96.46%> (-2.97%) ⬇️
elastica/wrappers/constraints.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1c008ba...c629a78. Read the comment docs.

Copy link
Contributor

@armantekinalp armantekinalp left a comment

Choose a reason for hiding this comment

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

I think everything looks good. I have some questions, could you please address them @skim0119

elastica/boundary_conditions.py Outdated Show resolved Hide resolved
elastica/boundary_conditions.py Outdated Show resolved Hide resolved
elastica/boundary_conditions.py Show resolved Hide resolved
Copy link
Contributor

@armantekinalp armantekinalp left a comment

Choose a reason for hiding this comment

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

LGTM, one small documentation comment.

elastica/boundary_conditions.py Outdated Show resolved Hide resolved
elastica/boundary_conditions.py Show resolved Hide resolved
@skim0119 skim0119 force-pushed the wip_constraint branch 2 times, most recently from 3d89597 to d837a80 Compare February 12, 2022 04:13
@skim0119 skim0119 merged commit a263a18 into update-0.2.2 Feb 12, 2022
@skim0119 skim0119 deleted the wip_constraint branch February 12, 2022 04:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request update Update the codebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants