Update and generalize fixed rod BC

[nmnaughton]

This partially addresses one of the comments in #36 regarding the OneEndFixedRod BC and also works to generalize the BC overall. There are three parts to the change.

1. In the constraints wrapper, currently the 'constrained_position_idx' argument gets popped from the kwargs dict. If we change this to get then this idx argument continues onto the boundary condition and allows the boundary condition to associate the position and directors with specific idx in the rod. @tp5uiuc I think you coded this originally, is there a reason we should be aware of why using get instead of pop would be advised?

2. Using the additional arguments from (1), any combination of nodes and elements can now be fixed in place by associating the idx and positions/directors info. Currently, I have added changes to OneEndFixedRod BC and also created an identical FixedRodBC. This is likely not the best solution. I am interested in hearing how people think we should proceed. Do we want to depreciate OneEndFixedRod? Is there a better naming scheme we could use?

3. I also included a separate BC (FixedNodeBC) where only the node is fixed. This would be a 'pinned' BC where the rod can still rotate around the point.

The most useful part of this change (to me at least) is that one can now fix either end (or both) of a rod.

I think one question for discussion is what should the name of the BCs be. Currently, there are OneEndFixedRod, FixedRodBC, and FixedNodeBC. I like FixedRodBC and FixedNodeBC, but is there a better name for FixedRodBC ? It should idealy denote that both an element and node is bing fixed. Maybe FixedSectionBC? Not sure...

[skim0119]

@nmnaughton Could you redirect the PR to update-0.3.0, and remove the branch update_fixed_BC? I think any change in this PR should be included in the next version release. If you want a faster update, we can create update-0.2.2 instead.

https://github.com/GazzolaLab/PyElastica/blob/master/CONTRIBUTING.md#project-workflow

[skim0119]

@nmnaughton Hi, 👋 I'm going over this PR, and it appears to me that there is more than one place to make the modification, especially because the change involves the backend. I understand your opinions above and I agree with your points, but we should be careful in modifying the constraint part.

A couple of identified issues:

• The design choice of the wrapper and finalize operation is to distinguish the model definition from execution. The indices are not supposed to be visible in BC. (Probably a design choice. We should double check if it is still necessary.)
• get->pop: We specifically use pop to ensure the number of given indices for position and director match (line 197 in constraint wrapper).
  • Hence, _kwargs is not passed to BC. In fact, both _args and _kwargs must be empty to run successfully, Otherwise, the above error would appear. (I understand it is a strange way.)

Here is my suggestion. ☕

• Save the branch and code in your fork for now. The new BC implementations look nice, and we should use them.
• Make the git-issue with the above problems that you would like to address.
• We can discuss the details on what is the best way to implement it. I agree with you that the current implementation of constraint does not allow us to specify which element to constraint.

[nmnaughton]

Hi @skim0119, for the issues you raised

1. The point of this PR is to address precisely this fact. While perhaps a design choice, I believe it was a mistake as it requires the BC to be hardcoded regarding the index. Any displacement BC inherently requires knowledge of which index the BC is being applied to, so we need to pass that info on somewhere. While the 2 displacement BC we have implemented (Helical and fixed end) do not need this idx info, any generalized class of displacement BC will. I regularly run into this issue and have to write my own BC to bypass this.

2. We currently drop fixed_element_idx and fixed_directors_collection from the arguments list, why don't we just make them required in init? If you don't need them, you don't have to use them (for example, the Helical BC doesn't need to), but then they are available as needed. I have a commit I can push later that implements this approach and it seems like a clean solution. While that would be a breaking change for any custom BC people have, we could easily handle it using a depreciation notice.

[skim0119]

1. The point of this PR is to address precisely this fact. While perhaps a design choice, I believe it was a mistake as it requires the BC to be hardcoded regarding the index. Any displacement BC inherently requires knowledge of which index the BC is being applied to, so we need to pass that info on somewhere. While the 2 displacement BC we have implemented (Helical and fixed end) do not need this idx info, any generalized class of displacement BC will. I regularly run into this issue and have to write my own BC to bypass this.

Yes, I agree. This part of the user interface needs an update.

2. We currently drop fixed_element_idx and fixed_directors_collection from the arguments list, why don't we just make them required in init? If you don't need them, you don't have to use them (for example, the Helical BC doesn't need to), but then they are available as needed. I have a commit I can push later that implements this approach and it seems like a clean solution. While that would be a breaking change for any custom BC people have, we could easily handle it using a depreciation notice.

The issue is that init is not really what the user is exposed to, instead users are supposed to use 'using'. I think the biggest mistake here is that the name constrained_position_idx sounds like we give indices where the constraints are happening, but the actual constraint definition doesn't depend on that. 😭

Sugestion ☕

How about we change the syntax such that if you pass some arbitrary index that is not named constrained_position(director)_idx, they are passed to init. You can use and save them for later. I think it is somewhat along the line of what you are suggesting. For example,

class CustomBC(FreeRod):
    def __init__(self, middle_indices:list[int]):
        FreeRod.__init__(self)
        self.middle_indices = middle_indices
        self.fixed_positions = self.get_system.position_collection[:, middle_indices]

    def constrain_values(self, rod, time): # Maybe rod can be replaced to self.get_system later
        rod.position_collection[:, middle_indices] = self.fixed_positions
        ...

...

simulator.constrain(rod).using(
    CustomBC,
    middle_indices=(6,7)
)

In this way, we don't need any deprecation warnings, as the previous features are all preserved. Passing arbitrary index is probably working even in the current version, but we just need self.get_system functionality.

If you already have a suggestion implemented as code, please link them 😄.

[skim0119]

After talking to @nmnaughton and @tp5uiuc, I decided to change the current structure of constraint, so that indices are passed to the constraint. It looks like this feature was intentionally removed in the past, but it probably makes more sense to bring it back. Ill make the modification and include in the next version.

[skim0119]

I'm merging these commits into wip_constraint. I'm going to continue on this problem.