BAMresearch / FenicsXConcrete

MIT License
0 stars 2 forks source link

84 - add units to time input #116

Closed aradermacher closed 1 year ago

aradermacher commented 1 year ago

my suggetsion to deal with the time stuff:

if we agree on that way, the sensors have to be updated (get rid of t input), as well as the examples have to be revised (get rid of t input in solve and pv_plot)

srosenbu commented 1 year ago

How is the user supposed to use these methods? According to your test, the user is supposed to call problem.update_time() after every solve. To me this sounds like something that should be done automatically.

I would suggest, the following structure for solve

#Suggestion 1: Optional timestep
class BaseMaterial:
    def solve(del_t: pint.Quantity | None = None):
        self.set_timestep(del_t) #set_timestep would need to accept None 
        # do stuff here
        self.update_time()

#Suggestion 2: 
class BaseMaterial:
    def solve():
        # do stuff here
        self.update_time()

I don't have any strong feelings about whether the timestep should be optional or not. In most of my cases, I either have a fixed timestep or I will do adaptive timestepping which will be handled by the Material class and not by the user. I don't know how we would enforce the time update in the solve, but it should be easy enough to have a unit test for that.

eriktamsen commented 1 year ago

I agree with Sjard, we should keep the necessary function calls for the user ot a minimum. I would suggest to agree on an implementation, that allows for maximum felixiblity in the future. I am sure things like automatic time and or load stepping can be integrated within fenicsXconcrete but are not a priority at the moment.

I dont see a (current) need to include the timestep in the solve call, so I would obt for simplicty now. However an extension, like the first suggestion can still be added later if needed, without breaking compatibility.

I would not worry too much about the paraview thing. I dont think the current implementation is the best we can do. For now we could always do a t-dt for paraview, till we implement it in a better way.

My question now would be, what is be most consistent way to include the timestep in the setup? 1) pass it as a parameter, like all the other things we currenty setup? 2) give an additional attribute to the problem object? 3) require to call a seperate "set_timestep" function? What do you think, @aradermacher, @srosenbu

aradermacher commented 1 year ago

Then I would prefer to use the parameter dict we already have for defining dt as pint.Quantity and use Sjards suggestion 2 or 1 without dt input doesn't matter because you have to reimplement that in your class