nemocrys / pyelmer

A python interface to Elmer.
GNU General Public License v3.0
56 stars 18 forks source link

Automatic Boundary ID Generation for Enhanced Boundary Control #36

Closed tapegoji closed 3 months ago

tapegoji commented 3 months ago

Description: This pull request introduces a enhancement to the pyelmer library by implementing automatic generation of boundary IDs during the initialization of boundaries, rather than when generating the sif file. This new feature offers the following benefits:

Enhanced Control: Users can now define relationships between different boundaries with greater ease. For instance, a mortar boundary condition (BC) can be set to depend on another mortar BC, providing more flexibility in complex simulations. Improved Usability: The automatic generation of boundary IDs simplifies the setup process, reducing the potential for user error and streamlining the workflow. Backward Compatibility: Existing functionality remains unaffected, ensuring seamless integration with current user projects. This update aims to improve the user experience and expand the capabilities of pyelmer for handling advanced boundary condition scenarios.

Please review the changes and provide feedback. Thank you for your support in making pyelmer better!

arvedes commented 3 months ago

Thanks for your contribution! When I started implementing pyelmer, I thought that assigning the IDs when the sif-file is written would be the safest way to ensure that there are no missing entries in the numbering of the BCs (and equations, solvers,...). This could happen now, e.g., when a user first creates a BC and then removes it later. But I don't think this is how people usually work with pyelmer, so I don't think this would cause many problems. Maybe a check would suffice.

Although I don't use mortar BCs in my simulations, I encountered similar problems when defining relations between different BCs for radiation. I implemented this workaround to establish relations between BCs, bodies, etc.:

import pyelmer.elmer as elmer
# (...)
bc_2.data.update({"Radiation Target Body": elmer.StringFromList([bc_1])})

The StringFromList function ensures that the ID is evaluated when the sif file is written and also works with a list of multiple BCs, though it's a somewhat dirty solution that people must know about to use it. What do you think about it? Would it also work for the relations between the mortar BCs?

I am unsure how to proceed, as the proposed change would make the code inconsistent. I think there are the following options:

  1. Keep it as it is; use StringFromList for such cases.
  2. Improve the code differently so that StringFromList is not needed (not sure how this could be done, though), but keep the assignment of the IDs the way they are.
  3. Change it for all equations, solvers, ... to the proposed way of assigning the IDs when the objects are created.

What do you think? Is there anyone else out there who has an opinion on that?

tpgillam commented 3 months ago

Just adding a couple of thoughts!

I happened to be thinking about this a few days ago, as we use pyelmer with mortar & periodic BCs. Currently:

I agree with @arvedes that it could become a little hard to follow if the semantics were different for boundaries and the other entities.

The StringFromList function ensures that the ID is evaluated when the sif file is written

This is handy; I wasn't aware this existed. It seems to be a good solution to the problem within the current design. I'll give it a go in our codebase!

As such I think proposal 1 works well. This approach also means that we can (or already will?) verify that the boundary we're referring to is actually in the simulation. With raw ids we'll just get subtle breakage later in Elmer!

Having said that, 3 is interesting, and it's simpler from the perspective of just having a simple wrapper around SIF. But it's quite a large design change IMO.

tapegoji commented 3 months ago

I added only the bcs this way. I was hoping if you all like it we can make the other ones the same way.

arvedes commented 3 months ago

I don't know if this is the way to go. I imagine a case where, e.g., there are many solvers or equations that need to be in a specific order. Sorting them would be helpful in such a case, which is possible with the current approach but would cause more trouble with the new one.

I am not really against changing it; I just don't see a big reason for such a drastic design change, and we possibly lose some functionality.

tapegoji commented 3 months ago

Okay, that sounds good. I will retract the pull request.